<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html;
      charset=windows-1252">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    Hi Shashi,<br>
    <br>
    Please see my comments inline:<br>
    <br>
    <div class="moz-cite-prefix">On 21/09/2018 23:22, Shashidhara
      Veerabhadraiah wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:b9b6bb78-3e3c-4924-92a6-026943e5b79c@default">
      <meta http-equiv="Content-Type" content="text/html;
        charset=windows-1252">
      <meta name="Generator" content="Microsoft Word 15 (filtered
        medium)">
      <style><!--
/* Font Definitions */
@font-face
        {font-family:Wingdings;
        panose-1:5 0 0 0 0 0 0 0 0 0;}
@font-face
        {font-family:PMingLiU;
        panose-1:2 1 6 1 0 1 1 1 1 1;}
@font-face
        {font-family:Tunga;
        panose-1:0 0 4 0 0 0 0 0 0 0;}
@font-face
        {font-family:"Cambria Math";
        panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
        {font-family:Calibri;
        panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
        {font-family:Consolas;
        panose-1:2 11 6 9 2 2 4 3 2 4;}
@font-face
        {font-family:"\@PMingLiU";}
@font-face
        {font-family:"Times New Roman \,serif";
        panose-1:0 0 0 0 0 0 0 0 0 0;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
        {margin:0in;
        margin-bottom:.0001pt;
        font-size:11.0pt;
        font-family:"Calibri",sans-serif;
        color:black;}
a:link, span.MsoHyperlink
        {mso-style-priority:99;
        color:#0563C1;
        text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
        {mso-style-priority:99;
        color:#954F72;
        text-decoration:underline;}
p
        {mso-style-priority:99;
        mso-margin-top-alt:auto;
        margin-right:0in;
        mso-margin-bottom-alt:auto;
        margin-left:0in;
        font-size:11.0pt;
        font-family:"Calibri",sans-serif;
        color:black;}
code
        {mso-style-priority:99;
        font-family:"Courier New";}
pre
        {mso-style-priority:99;
        mso-style-link:"HTML Preformatted Char";
        margin:0in;
        margin-bottom:.0001pt;
        font-size:10.0pt;
        font-family:"Courier New";
        color:black;}
p.MsoListParagraph, li.MsoListParagraph, div.MsoListParagraph
        {mso-style-priority:34;
        margin-top:0in;
        margin-right:0in;
        margin-bottom:0in;
        margin-left:.5in;
        margin-bottom:.0001pt;
        font-size:11.0pt;
        font-family:"Calibri",sans-serif;
        color:black;}
span.HTMLPreformattedChar
        {mso-style-name:"HTML Preformatted Char";
        mso-style-priority:99;
        mso-style-link:"HTML Preformatted";
        font-family:Consolas;
        color:black;}
span.new
        {mso-style-name:new;}
span.membernamelink
        {mso-style-name:membernamelink;}
span.changed
        {mso-style-name:changed;}
span.EmailStyle25
        {mso-style-type:personal;
        font-family:"Calibri",sans-serif;
        color:windowtext;}
span.EmailStyle26
        {mso-style-type:personal;
        font-family:"Calibri",sans-serif;
        color:#1F497D;}
span.EmailStyle27
        {mso-style-type:personal;
        font-family:"Calibri",sans-serif;
        color:windowtext;}
span.EmailStyle28
        {mso-style-type:personal;
        font-family:"Calibri",sans-serif;
        color:#1F497D;}
span.EmailStyle29
        {mso-style-type:personal;
        font-family:"Calibri",sans-serif;
        color:windowtext;}
span.EmailStyle30
        {mso-style-type:personal-reply;
        font-family:"Calibri",sans-serif;
        color:#1F497D;}
.MsoChpDefault
        {mso-style-type:export-only;
        font-size:10.0pt;}
@page WordSection1
        {size:8.5in 11.0in;
        margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
        {page:WordSection1;}
/* List Definitions */
@list l0
        {mso-list-id:901988770;
        mso-list-template-ids:1374585078;}
@list l0:level1
        {mso-level-number-format:bullet;
        mso-level-text:\F0B7;
        mso-level-tab-stop:.5in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Symbol;}
@list l0:level2
        {mso-level-number-format:bullet;
        mso-level-text:o;
        mso-level-tab-stop:1.0in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:"Courier New";
        mso-bidi-font-family:"Times New Roman";}
@list l0:level3
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:1.5in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l0:level4
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:2.0in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l0:level5
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:2.5in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l0:level6
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:3.0in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l0:level7
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:3.5in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l0:level8
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:4.0in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l0:level9
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:4.5in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l1
        {mso-list-id:1398866647;
        mso-list-template-ids:864182752;}
@list l1:level1
        {mso-level-number-format:bullet;
        mso-level-text:\F0B7;
        mso-level-tab-stop:.5in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Symbol;}
@list l1:level2
        {mso-level-number-format:bullet;
        mso-level-text:o;
        mso-level-tab-stop:1.0in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:"Courier New";
        mso-bidi-font-family:"Times New Roman";}
@list l1:level3
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:1.5in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l1:level4
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:2.0in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l1:level5
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:2.5in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l1:level6
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:3.0in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l1:level7
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:3.5in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l1:level8
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:4.0in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l1:level9
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:4.5in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l2
        {mso-list-id:1511217915;
        mso-list-template-ids:1018584036;}
@list l2:level1
        {mso-level-number-format:bullet;
        mso-level-text:\F0B7;
        mso-level-tab-stop:.5in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Symbol;}
@list l2:level2
        {mso-level-number-format:bullet;
        mso-level-text:o;
        mso-level-tab-stop:1.0in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:"Courier New";
        mso-bidi-font-family:"Times New Roman";}
@list l2:level3
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:1.5in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l2:level4
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:2.0in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l2:level5
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:2.5in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l2:level6
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:3.0in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l2:level7
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:3.5in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l2:level8
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:4.0in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l2:level9
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:4.5in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l3
        {mso-list-id:1867256503;
        mso-list-template-ids:971115580;}
@list l3:level1
        {mso-level-number-format:bullet;
        mso-level-text:\F0B7;
        mso-level-tab-stop:.5in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Symbol;}
@list l3:level2
        {mso-level-number-format:bullet;
        mso-level-text:o;
        mso-level-tab-stop:1.0in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:"Courier New";
        mso-bidi-font-family:"Times New Roman";}
@list l3:level3
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:1.5in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l3:level4
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:2.0in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l3:level5
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:2.5in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l3:level6
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:3.0in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l3:level7
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:3.5in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l3:level8
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:4.0in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l3:level9
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:4.5in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l4
        {mso-list-id:1921719012;
        mso-list-template-ids:-1036637156;}
@list l4:level1
        {mso-level-number-format:bullet;
        mso-level-text:\F0B7;
        mso-level-tab-stop:.5in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Symbol;}
@list l4:level2
        {mso-level-number-format:bullet;
        mso-level-text:o;
        mso-level-tab-stop:1.0in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:"Courier New";
        mso-bidi-font-family:"Times New Roman";}
@list l4:level3
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:1.5in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l4:level4
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:2.0in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l4:level5
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:2.5in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l4:level6
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:3.0in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l4:level7
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:3.5in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l4:level8
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:4.0in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
@list l4:level9
        {mso-level-number-format:bullet;
        mso-level-text:\F0A7;
        mso-level-tab-stop:4.5in;
        mso-level-number-position:left;
        text-indent:-.25in;
        mso-ansi-font-size:10.0pt;
        font-family:Wingdings;}
ol
        {margin-bottom:0in;}
ul
        {margin-bottom:0in;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
      <div class="WordSection1">
        <p class="MsoNormal"><span style="color:#1F497D">Hi Alexey,
            Thanks for your review and below is the new Webrev.<o:p></o:p></span></p>
        <p class="MsoNormal"><span style="color:#1F497D"><a
              href="http://cr.openjdk.java.net/%7Esveerabhadra/8182043/webrev.03/"
              moz-do-not-send="true">http://cr.openjdk.java.net/~sveerabhadra/8182043/webrev.03/</a><o:p></o:p></span></p>
        <p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></p>
        <p class="MsoNormal"><span style="color:#1F497D">Please see
            below for inline comments.<o:p></o:p></span></p>
        <p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></p>
        <p class="MsoNormal"><span style="color:#1F497D">Thanks and
            regards,<br>
            Shashi<o:p></o:p></span></p>
        <p class="MsoNormal"><a name="_MailEndCompose"
            moz-do-not-send="true"><span style="color:#1F497D"><o:p> </o:p></span></a></p>
        <div>
          <div style="border:none;border-top:solid #E1E1E1
            1.0pt;padding:3.0pt 0in 0in 0in">
            <p class="MsoNormal"><b><span style="color:windowtext">From:</span></b><span
                style="color:windowtext"> Alexey Ivanov <br>
                <b>Sent:</b> Friday, September 21, 2018 2:09 PM<br>
                <b>To:</b> Shashidhara Veerabhadraiah
                <a class="moz-txt-link-rfc2396E" href="mailto:shashidhara.veerabhadraiah@oracle.com"><shashidhara.veerabhadraiah@oracle.com></a>; Prasanta
                Sadhukhan <a class="moz-txt-link-rfc2396E" href="mailto:prasanta.sadhukhan@oracle.com"><prasanta.sadhukhan@oracle.com></a>;
                swing-dev <a class="moz-txt-link-rfc2396E" href="mailto:swing-dev@openjdk.java.net"><swing-dev@openjdk.java.net></a>; awt-dev
                <a class="moz-txt-link-rfc2396E" href="mailto:awt-dev@openjdk.java.net"><awt-dev@openjdk.java.net></a><br>
                <b>Subject:</b> Re: <AWT Dev> <Swing Dev>
                [12] JDK-8182043: Access to Windows Large Icons<o:p></o:p></span></p>
          </div>
        </div>
        <p class="MsoNormal"><o:p> </o:p></p>
        <p class="MsoNormal" style="margin-bottom:12.0pt">Hi Shashi,<br>
          <br>
          SystemIcon.java<br>
          What is the purpose of new SystemIcon class?<br>
          It's not used anywhere but the provided test. Is this class
          really needed then?<br>
          Is it supposed to become the public API for accessing system
          icons?<br>
          Why can't FileSystemView be used for that purpose as it was
          proposed in Semyon's review?<br>
          <b><i><span style="color:#1F497D">[Shashi] </span></i></b><span
            style="color:#1F497D">SystemIcon is going to be the front
            face to access the icons and that is the purpose of this
            class. The reason for choosing this is that FileSystemView
            class can be used internally and did not wanted to expose it
            externally too. Externally exposing may cause certain
            restriction in maintaining the classes hence the
            indirection.</span><br>
        </p>
      </div>
    </blockquote>
    <br>
    Still, I cannot understand the rationale for a new class the only
    purpose of which is to provide public access to getSystemIcon(File,
    int, int).<br>
    FileSystemView is already a public class, and it's used internally.
    (I guess it would not have existed, if it hadn't.) It has a public
    method getSystemIcon(File). As such, extending its functionality to
    get access to larger icons seems logical. This is what the new
    protected getSystemIcon(File f, int size) does.<br>
    <br>
    It can be made public to facilitate access to file icons.<br>
    After all, protected method is also a contract, it cannot be changed
    without affecting backward compatibility.<br>
    <br>
    It is this new protected method that performs the task of getting
    the icon from the system.<br>
    <br>
    Do we really need other methods?<br>
    <br>
    <br>
    278     public File createNewFolder(File containingDir) throws
    IOException {<br>
    279         return null;<br>
    280     }<br>
    You had to implement the abstract method from FileSystemView. It's
    one more point to make system icon available right from
    FileSystemView.<br>
    This implementation should rather throw an exception.<br>
    <br>
    <br>
    60         protected File file;<br>
    This field is redundant, in my opinion. It would be quite expensive
    to instantiate SystemIcon object for each file. It can safely be
    removed, then only methods which take additional File parameter will
    be left.<br>
    (The field could be final as it cannot be changed and should not be
    changed.)<br>
    <br>
    65     public SystemIcon() {<br>
    66         this.file = null;<br>
    67     }<br>
    Should rather call this(null) constructor.<br>
    <br>
    112     public Icon getSystemIcon(int width, int height) {<br>
    Are methods with width / height parameters needed? Icons are usually
    square.<br>
    <br>
    You repeat checks if f is null, width and height checks in each and
    every method. I guess parameter validation could be extract into a
    separate method. You will avoid lots of cope duplication.<br>
    <br>
    Since it's a completely new API, I suggest throwing
    IllegalArgumentException with appropriate message in the cases where
    a parameter (file, width and height) fails validation.<br>
    <br>
    210         int size;<br>
    211         if(width > height) {<br>
    212             size = width;<br>
    213         } else {<br>
    214             size = height;<br>
    215         }<br>
    This code can be simplified to<br>
    int size = Math.max(width, height);<br>
    Concise and clear.<br>
    A helper method which validates the parameters could also return
    this value. Thus, again, avoiding code duplication among many
    methods in this class.<br>
    <br>
    <br>
    There are lots of tabs in this file. Tabs must be replaced with
    spaces.<br>
    if's are inconsistent throughout the code: some are with space, some
    are without. Please add the space everyone to align with Java Code
    Conventions.<br>
    Please also sort the imports and remove unused ones.<br>
    <br>
    <br>
    <blockquote type="cite"
      cite="mid:b9b6bb78-3e3c-4924-92a6-026943e5b79c@default">
      <div class="WordSection1">
        <p class="MsoNormal" style="margin-bottom:12.0pt">FileSystemView.java<br>
           259      * Icon for a file, directory, or folder as it would
          be displayed in<br>
           260      * a system file browser for the requested size.<br>
          For getXXX, it's better to start description with “Returns…”
          so it aligns to other similar methods.<br>
          However, I see the new method follows description of
          getIcon(boolean).<span style="color:#1F497D"><o:p></o:p></span></p>
        <p class="MsoNormal" style="margin-bottom:12.0pt"><b><i><span
                style="color:#1F497D">[Shashi] </span></i></b><span
            style="color:#1F497D">Because as you said rightly it follows
            the getIcon(boolean)</span><br>
        </p>
      </div>
    </blockquote>
    <br>
    Okay.<br>
    Is it possible to update documentation to the existing
    getSystemIcon(File)?<br>
    Should I file a separate bug to update the documentation?<br>
    <br>
    Documentation also references a non-public class ShellFolder. Should
    this reference be removed from documentation as the access to
    non-public classes is restricted? It does not add much value.<br>
    <br>
    <blockquote type="cite"
      cite="mid:b9b6bb78-3e3c-4924-92a6-026943e5b79c@default">
      <div class="WordSection1">
        <p class="MsoNormal" style="margin-bottom:12.0pt"> 265      *
          @param size width and height of the icon in pixels to be
          scaled(valid range: 1 to 256)<br>
          Why is it “to be scaled”? I would expect to get the icon of
          the requested size. At the same time, the icon can be scaled
          to the requested size if the requested size is not available.<span
            style="color:#1F497D"><o:p></o:p></span></p>
        <p class="MsoNormal" style="margin-bottom:12.0pt"><b><i><span
                style="color:#1F497D">[Shashi] </span></i></b><span
            style="color:#1F497D">User has no restriction of mentioning
            any size but the platform may have a limitation of size.
            Since we are supporting a set of different versions of
            platforms, platform may limit the size of the icon to a
            particular size, in which case it will be scaled to the user
            requested size.</span><br>
        </p>
      </div>
    </blockquote>
    <br>
    I understand that. However, I think the suggested description does
    not convey the meaning correctly.<br>
    The method will return the icon of the requested size, won't it?<br>
    So the correct description is:<br>
    @param size width and height of the icon in pixels (valid range: 1
    to 256)<br>
    <br>
    The fact the returned icon may be scaled if the requested size is
    not available must be described in the method documentation as well
    as in @return line:<br>
    @return an icon of the requested size (possibly scaled) as it would
    be displayed by a native file chooser<br>
    <br>
    <blockquote type="cite"
      cite="mid:b9b6bb78-3e3c-4924-92a6-026943e5b79c@default">
      <div class="WordSection1">
        <p class="MsoNormal" style="margin-bottom:12.0pt">270    
          protected Icon getSystemIcon(File f, int size) {<br>
          Can't the method be public? It was in Semyon's review.<span
            style="color:#1F497D"><o:p></o:p></span></p>
        <p class="MsoNormal" style="margin-bottom:12.0pt"><b><i><span
                style="color:#1F497D">[Shashi] </span></i></b><span
            style="color:#1F497D">Because of the indirection, this
            method can stay as protected. I think it is always good to
            be of using protected than making everything public. Also
            that is the advantage of adding the SystemIcon class.</span></p>
      </div>
    </blockquote>
    <br>
    Sorry I don't see any advantage of having SystemIcon class over
    making this method public as I outlined above.<br>
    <br>
    <blockquote type="cite"
      cite="mid:b9b6bb78-3e3c-4924-92a6-026943e5b79c@default">
      <div class="WordSection1">
        <p class="MsoNormal" style="margin-bottom:12.0pt"> 266      *
          @return an icon as it would be displayed by a native file
          chooser<br>
          An icon of the requested size (possibly scaled) as…<br>
          <br>
           275         if(size > 256 || size < 1) {<br>
           276             return null;<br>
           277         }<br>
          Please add space between if and the opening parenthesis.<br>
          You can throw InvalidArgumentException in this case.<br>
          Does size of 1 make any sense?<span style="color:#1F497D"><o:p></o:p></span></p>
        <p class="MsoNormal" style="margin-bottom:12.0pt"><b><i><span
                style="color:#1F497D">[Shashi] </span></i></b><span
            style="color:#1F497D">Done. I can only say that 0 does not
            make sense. Check is to see that it is not less than 1.</span><br>
        </p>
      </div>
    </blockquote>
    <br>
    What about throwing InvalidArgumentException when size parameter is
    invalid?<br>
    <br>
    I understand that check is to make sure size is at least 1. However,
    icon of 1 pixel size does not make any sense. Should the minimum be
    a more sensible of 4?<br>
    It's a concern for discussion.<br>
    <br>
    <blockquote type="cite"
      cite="mid:b9b6bb78-3e3c-4924-92a6-026943e5b79c@default">
      <div class="WordSection1">
        <p class="MsoNormal" style="margin-bottom:12.0pt"><br>
          <br>
          ShellFolder.java<br>
           202     /**<br>
           203      * @param size size of the icon > 0<br>
           204      * @return The icon used to display this shell folder<br>
           205      */<br>
          Can you add a short description of the purpose of this method?
          “Returns the icon of the specified size used to display this
          shell folder”?<br>
          A similar description can be added to the method above it:<br>
          198     public Image getIcon(boolean getLargeIcon) {<span
            style="color:#1F497D"><o:p></o:p></span></p>
        <p class="MsoNormal" style="margin-bottom:12.0pt"><b><i><span
                style="color:#1F497D">[Shashi] </span></i></b><span
            style="color:#1F497D">Updated. Thank you.</span><br>
        </p>
      </div>
    </blockquote>
    <br>
    Thank you for updating @return clause of the Javadoc.<br>
    My intention was to add a generic description of the method as well:<br>
    202     /**<br>
    202      * Returns the icon of the specified size used to display
    this shell folder.<br>
    202      *<br>
    203      * @param size size of the icon > 0<br>
    204      * @return The icon used to display this shell folder<br>
    205      */<br>
    <br>
    Such description could also be added to method above getIcon(boolean
    getLargeIcon), at line 198.<br>
    <br>
    Should the range of size parameter be specified? For example, 1–256
    as in FileSystemView.<br>
    <br>
    <blockquote type="cite"
      cite="mid:b9b6bb78-3e3c-4924-92a6-026943e5b79c@default">
      <div class="WordSection1">
        <p class="MsoNormal" style="margin-bottom:12.0pt">ShellFolder2.cpp<br>
           944             hres = pIcon->Extract(szBuf, index,
          &hIcon, 0, size);<br>
          Please use NULL instead of 0. This way it's clear you pass a
          null pointer rather an integer with value of 0.<span
            style="color:#1F497D"><o:p></o:p></span></p>
        <p class="MsoNormal" style="margin-bottom:12.0pt"><b><i><span
                style="color:#1F497D">[Shashi] </span></i></b><span
            style="color:#1F497D">Updated.</span><b><i><span
                style="color:#1F497D"><o:p></o:p></span></i></b></p>
        <p class="MsoNormal" style="margin-bottom:12.0pt"><br>
          <br>
          974     const int MAX_ICON_SIZE = 128;<br>
          I also suggest increasing MAX_ICON_SIZE to 256. Otherwise I
          see no point in allowing 256 as the maximum size at Java level
          as you'll never have icon of 256×256 even thought the system
          may have one.<b><i><span style="color:#1F497D"><o:p></o:p></span></i></b></p>
        <p class="MsoNormal" style="margin-bottom:12.0pt"><b><i><span
                style="color:#1F497D">[Shashi] </span></i></b><span
            style="color:#1F497D">Per me, the problem is that since we
            support certain older versions of the platforms, it should
            not cause an exception at the native level. If everyone
            agrees for the change then we can change that.</span><br>
        </p>
      </div>
    </blockquote>
    <br>
    This concern was raised in the previous review too:<br>
<a class="moz-txt-link-freetext" href="http://mail.openjdk.java.net/pipermail/awt-dev/2017-September/013115.html">http://mail.openjdk.java.net/pipermail/awt-dev/2017-September/013115.html</a><br>
    <br>
    I think it's safe to update the value of MAX_ICON_SIZE to 256. The
    oldest supported version of Windows is Windows 7 which supports
    256×256 icons.<br>
    Windows XP used icons up to 48×48, but it does not imply the API
    does not allow loading icon of larger size. Both 128 and 256 should
    be tested on Windows XP if JDK still runs on it.<br>
    <br>
    <blockquote type="cite"
      cite="mid:b9b6bb78-3e3c-4924-92a6-026943e5b79c@default">
      <div class="WordSection1">
        <p class="MsoNormal" style="margin-bottom:12.0pt"><br>
          <br>
          Win32ShellFolder2.java<br>
          1007     private static Image makeIcon(long hIcon, int bsize)
          {<br>
          bsize has no meaning. Prasanta also asked about the name of
          the parameter.<br>
          I suggest using "size" for parameter, and "iconSize" for local
          variable.<span style="color:#1F497D"><o:p></o:p></span></p>
        <p class="MsoNormal" style="margin-bottom:12.0pt"><b><i><span
                style="color:#1F497D">[Shashi] </span></i></b><span
            style="color:#1F497D">Updated.<o:p></o:p></span></p>
        <p class="MsoNormal" style="margin-bottom:12.0pt"><br>
          <br>
          1031         int size = getLargeIcon ? 32 : 16; // large icon
          is 32 pixels else 16 pixels<br>
          Create constants for these magic numbers.<br>
          <a
href="http://cr.openjdk.java.net/%7Essadetsky/8182043/webrev.01/src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java.udiff.html"
            moz-do-not-send="true">http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.01/src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java.udiff.html</a><span
            style="color:#1F497D"><o:p></o:p></span></p>
        <p class="MsoNormal" style="margin-bottom:12.0pt"><b><i><span
                style="color:#1F497D">[Shashi] </span></i></b><span
            style="color:#1F497D">Updated.<o:p></o:p></span></p>
        <p class="MsoNormal" style="margin-bottom:12.0pt"><br>
          <br>
          1080                                         return
          getShell32Icon(4, size); // pick folder icon<br>
          1081                                     } else {<br>
          1082                                         return
          getShell32Icon(1, size); // pick file icon<br>
          Also create constants for 4 and 1 as Prasanta suggested.<br>
          Creating a constant costs you nothing but makes the code more
          readable without adding comments.<span style="color:#1F497D"><o:p></o:p></span></p>
        <p class="MsoNormal" style="margin-bottom:12.0pt"><b><i><span
                style="color:#1F497D">[Shashi] </span></i></b><span
            style="color:#1F497D">But my view is that if we start doing
            for every immediate value, then creation of the object and
            assignment may have some implications. If it is used more
            than once, yes definitely create the constant. Please let me
            know if you think otherwise. This I follow it as a standard
            by practice. </span></p>
      </div>
    </blockquote>
    <br>
    Primitive type constants do not have any performance penalty as
    they're resolved at compile time.<br>
    There's no difference if you write getShell32Icon(FOLDER_ICON_INDEX,
    size) or getShell32Icon(4, size). Yet the former is much clearer,
    isn't it?<br>
    <br>
    With the constants, the if expression could be replaced with the
    conditional operator:<br>
    <br>
    if(hIcon <= 0) {<br>
        return getShell32Icon(isDirectory() ? FOLDER_ICON_INDEX :
    FILE_ICON_INDEX, size);<br>
    }<br>
    <br>
    <br>
    I agree not every number in code should be defined as a constant.
    Yet I'm for using constants in this particular piece of code.<br>
    <br>
    These values are used at least twice in Win32ShellFolder2.java:
    lines 1081–1085 and 1119–1123.<br>
    <br>
    <br>
    1138         Image icon = makeIcon(hIcon, 32);<br>
    Now should use LARGE_ICON_SIZE. <br>
    <br>
    <blockquote type="cite"
      cite="mid:b9b6bb78-3e3c-4924-92a6-026943e5b79c@default">
      <div class="WordSection1">
        <p class="MsoNormal" style="margin-bottom:12.0pt">1113            
          if(hIcon <= 0) {<br>
          1116                 if(hIcon <= 0) {<br>
          Please add space between if and the opening parenthesis.<span
            style="color:#1F497D"><o:p></o:p></span></p>
        <p class="MsoNormal" style="margin-bottom:12.0pt"><b><i><span
                style="color:#1F497D">[Shashi]</span></i></b><span
            style="color:#1F497D"> Updated.</span><br>
        </p>
      </div>
    </blockquote>
    <br>
    1078                             if(hIcon <= 0) {<br>
    1081                                 if(hIcon <= 0) {<br>
    These (in the method above) could also be updated.<br>
    <br>
    <blockquote type="cite"
      cite="mid:b9b6bb78-3e3c-4924-92a6-026943e5b79c@default">
      <div class="WordSection1">
        <p class="MsoNormal" style="margin-bottom:12.0pt">Win32ShellFolderManager2.java<br>
           382                     return
          Win32ShellFolder2.getShell32Icon(i,
          key.startsWith("shell32LargeIcon ")?<br>
 383                                                                 32
          : 16);<br>
          You can use constants declared for icon size in from
          Win32ShellFolder2 because they're already imported:<br>
            42 import static sun.awt.shell.Win32ShellFolder2.*;<span
            style="color:#1F497D"><o:p></o:p></span></p>
        <p class="MsoNormal" style="margin-bottom:12.0pt"><b><i><span
                style="color:#1F497D">[Shashi] </span></i></b><span
            style="color:#1F497D">Updated.</span></p>
      </div>
    </blockquote>
    <br>
     382                     return Win32ShellFolder2.getShell32Icon(i,
    key.startsWith("shell32LargeIcon ")?<br>
     383                                                                
    LARGE_ICON_SIZE : SMALL_ICON_SIZE);<br>
    <br>
    May I suggest updating formatting to:<br>
                        return Win32ShellFolder2.getShell32Icon(i,<br>
                                key.startsWith("shell32LargeIcon ") ?
    LARGE_ICON_SIZE : SMALL_ICON_SIZE);<br>
    or even<br>
                        return Win32ShellFolder2.getShell32Icon(i,<br>
                                key.startsWith("shell32LargeIcon ") ?
    LARGE_ICON_SIZE<br>
                                                                    :
    SMALL_ICON_SIZE);<br>
    (where : aligns with ?)<br>
    <br>
    <blockquote type="cite"
      cite="mid:b9b6bb78-3e3c-4924-92a6-026943e5b79c@default">
      <div class="WordSection1">
        <p class="MsoNormal" style="margin-bottom:12.0pt"><span
            style="color:#1F497D"><o:p></o:p></span><br>
          Then the code at these lines needs to be updated too:<br>
           129             STANDARD_VIEW_BUTTONS[iconIndex] = (size ==
          16)<br>
           130                     ? img<br>
           131                     : new MultiResolutionIconImage(16,
          img);<br>
          <br>
          See <a
href="http://cr.openjdk.java.net/%7Essadetsky/8182043/webrev.01/src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolderManager2.java.udiff.html"
            moz-do-not-send="true">http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.01/src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolderManager2.java.udiff.html</a><br>
          <br>
          <br>
          SystemIconTest.java<br>
          <br>
          Could you please order the imports? Your IDE would be happy to
          do it for you.<br>
          Could you also add spaces between if and the opening
          parenthesis?<br>
          (Or at least be consistent with it.)<br>
          <br>
          58             ImageIcon icon =
          (ImageIcon)sysIcon.getSystemIcon(file, size, size);<br>
          Casts should be followed by a blank space.<br>
          <br>
          67             else if (icon.getIconWidth() != size) {<br>
          else is redundant as the preceding if throws an exception.<span
            style="color:#1F497D"><o:p></o:p></span></p>
        <p class="MsoNormal" style="margin-bottom:12.0pt"><b><i><span
                style="color:#1F497D">[Shashi] </span></i></b><span
            style="color:#1F497D">Updated.</span></p>
      </div>
    </blockquote>
    <br>
    Could you please organize imports?<br>
    There are only three classes used.<br>
    <br>
    41             System.out.println("Windows detected: will run sytem
    icons test");<br>
    typo: system<br>
    <br>
    Since the test is Windows-specific, it can be declared using
    @requires tag of JTreg:<br>
    @requires os.family == "windows"<br>
    <br>
    <br>
    Regards,<br>
    Alexey<br>
    <br>
    <blockquote type="cite"
      cite="mid:b9b6bb78-3e3c-4924-92a6-026943e5b79c@default">
      <div class="WordSection1">
        <p class="MsoNormal" style="margin-bottom:12.0pt"><span
            style="color:#1F497D"><o:p></o:p></span></p>
        <p class="MsoNormal" style="margin-bottom:12.0pt"><br>
          <br>
          <br>
          Regards,<br>
          Alexey<br>
          <br>
          <b><i><span style="color:#1F497D"><o:p></o:p></span></i></b></p>
        <div>
          <p class="MsoNormal">On 04/09/2018 10:39, Shashidhara
            Veerabhadraiah wrote:<o:p></o:p></p>
        </div>
        <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
          <p class="MsoNormal"><span style="color:#1F497D">Hi All,
              Please find the updated Webrev per the discussion:</span><o:p></o:p></p>
          <p class="MsoNormal"><span style="color:#1F497D"> </span><o:p></o:p></p>
          <p class="MsoNormal"><span style="color:#1F497D"><a
                href="http://cr.openjdk.java.net/%7Esveerabhadra/8182043/webrev.02/"
                moz-do-not-send="true">http://cr.openjdk.java.net/~sveerabhadra/8182043/webrev.02/</a></span><o:p></o:p></p>
          <p class="MsoNormal"><span style="color:#1F497D"> </span><o:p></o:p></p>
          <p class="MsoNormal"><span style="color:#1F497D">Thanks and
              regards,</span><o:p></o:p></p>
          <p class="MsoNormal"><span style="color:#1F497D">Shashi</span><o:p></o:p></p>
          <p class="MsoNormal"><span style="color:#1F497D"><br>
              <SNIP></span><br>
          </p>
        </blockquote>
      </div>
    </blockquote>
    <br>
  </body>
</html>