<div dir="auto"><div>Hi Phil,</div><div dir="auto"><br></div><div dir="auto">Few questions about this patch.</div><div dir="auto"><br><div dir="auto">I wonder if IcedTeaWeb will be broken by such change removing the support of multiple AppContexts in FontManager.</div><div dir="auto"><br></div>I would have prefered keeping maybeMultiAppContext() relying on a system property than removing all the internal mechanism, that could be maintained externally but would mean having a forked openjdk to keep this specific code.</div><div dir="auto"><br></div><div dir="auto">What are the possible side effects if this change is applied on application using multiple AppContexts ?</div><div dir="auto"><br></div><div dir="auto">Laurent<br><br><div class="gmail_quote" dir="auto"><div dir="ltr">Le mer. 30 janv. 2019 à 21:56, Phil Race <<a href="mailto:philip.race@oracle.com">philip.race@oracle.com</a>> a écrit :<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  
    
  
  <div text="#000000" bgcolor="#FFFFFF">
    The synopsis - which I rephrased for better English, turns out to
    still<br>
    not represent most of what is happening here. The synopsis would<br>
    make one think this is mostly about removing an unused abstract
    class<br>
    and the font changes are a foot note to that. In fact this is MOSTLY<br>
    about removing per-appcontext code in the font code - code which<br>
    just asked for an appcontext and had no direct relationship with the<br><span style="font-family:sans-serif">AWTSecurityManager</span> class except to check for it in a one time call
    as a cheap way<br>
    of knowing we needed to be prepared to check for multiple contexts.<br>
    If you are removing AWTSecurityManager, the logical thing would be<br>
    to find another way to see if it is multiple appcontexts, not remove
    all the code<br>
    that handles such a case.<br>
    In other words there is an extrapolation here from
    "AWTSecurityManager is gone"<br>
    to "multiple appcontexts are gone" which happens to be true, but
    does not<br>
    logically follow.<br>
    <br>
    So I thought I was reviewing one thing and it turns out to be quite
    different<br>
    and better summarised as :<br>
    "Remove multiple appcontext support from the font code because
    AWTSecurityManager is being removed"<br>
    The removal of AWTSecurityManager is < 10% of this change .. <br>
    <br>
    Alternatively everything changed in the font code could have been
    handled with this :-<br>
    <pre><span class="m_7990087570331560227changed">private static boolean maybeMultiAppContext() {
    // Do we have multiple app contexts any more ?
    // Or do we need a new way to check for multiple app contexts ?
    // If not, can we remove this code + its downstream dependencies too in a separate fix ?
    return false;
}


Are there any tests that are also obsoleted ?

I am approving this change (modulo the answer about tests) since the end result will
be the same but personally I would have split it into an AWT bug and a 2D bug each
with an appropriate synopsis.
</span></pre>
    -phil.<br>
    <br>
    <div class="m_7990087570331560227moz-cite-prefix">On 1/15/19 11:49 AM, Sergey Bylokhov
      wrote:<br>
    </div>
    <blockquote type="cite">Hello.
      <br>
      Please review the fix for jdk 13.
      <br>
      <br>
      Bug: <a class="m_7990087570331560227moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8216592" target="_blank" rel="noreferrer">https://bugs.openjdk.java.net/browse/JDK-8216592</a>
      <br>
      Webrev: <a class="m_7990087570331560227moz-txt-link-freetext" href="http://cr.openjdk.java.net/~serb/8216592/webrev.01" target="_blank" rel="noreferrer">http://cr.openjdk.java.net/~serb/8216592/webrev.01</a>
      <br>
      <br>
      The AWTSecurityManager class is an abstract class which provided
      <br>
      the AppContext objects through SecurityManager extensions. The
      plugin
      <br>
      provided a concrete implementation of this class. Since plugin was
      <br>
      removed the AWTSecurityManager itself can be removed as well.
      <br>
      <br>
      Since we currently never set AWTSecurityManager some of our checks
      like:
      <br>
      "sm instanceof sun.awt.AWTSecurityManager"
      <br>
      are always "false". And I dropped the code which uses such
      <br>
      checks(mostly in the SunFontManager)
      <br>
      <br>
      <br>
    </blockquote>
    <br>
  </div>

</blockquote></div></div></div>