One reason of Bug 87993 is that Internals objects don't follow typical WebCore lifetime model. It should follow either Document(or Window), Frame or Page.
Created attachment 146208 [details] Patch
This change is a part of reorg for addressing Bug 87933.
Created attachment 146215 [details] Patch
Comment on attachment 146215 [details] Patch Attachment 146215 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12913158
Comment on attachment 146215 [details] Patch Attachment 146215 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12906950 New failing tests: http/tests/inspector/extensions-headers.html http/tests/inspector/extensions-ignore-cache.html http/tests/inspector/console-xhr-logging.html http/tests/inspector/console-xhr-logging-async.html http/tests/inspector/console-cd.html http/tests/inspector/console-cd-completions.html http/tests/inspector/injected-script-for-origin.html http/tests/inspector/console-cross-origin-iframe-logging.html http/tests/inspector/network-preflight-options.html http/tests/inspector/extensions-network-redirect.html http/tests/appcache/crash-when-navigating-away-then-back.html http/tests/inspector/modify-cross-domain-rule.html http/tests/inspector/inspect-iframe-from-different-domain.html http/tests/inspector/change-iframe-src.html http/tests/inspector/compiler-script-mapping.html http/tests/inspector/extensions-useragent.html http/tests/inspector/compiler-source-mapping-debug.html http/tests/inspector/resource-har-conversion.html http/tests/inspector/resource-har-headers.html http/tests/inspector/console-resource-errors.html compositing/geometry/fixed-position-transform-composited-page-scale.html
Created attachment 146273 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 146215 [details] Patch I don't understand why Internals and InternalsController need to be separate.
(In reply to comment #7) > (From update of attachment 146215 [details]) > I don't understand why Internals and InternalsController need to be separate. This is because - Internals follows Document lifetime and holds document, that enable us to kill redudant Document* parameters from many Internals APIs in following changes. InternalsController needs to survive across document to keep InternalSettings alive. So it need to follow Page lifetime. - Supplement<T> cannot be RefCounted. But Internals should be.
(In reply to comment #8) > (In reply to comment #7) > > (From update of attachment 146215 [details] [details]) > > I don't understand why Internals and InternalsController need to be separate. > > This is because > > - Internals follows Document lifetime and holds document, > that enable us to kill redudant Document* parameters from many Internals APIs > in following changes. > InternalsController needs to survive across document to keep InternalSettings > alive. So it need to follow Page lifetime. > - Supplement<T> cannot be RefCounted. But Internals should be. I like the approach of this patch and I agree it allows us not only to fix https://bugs.webkit.org/show_bug.cgi?id=87993 but that it also enables us to find more room for future improvements.
> > I don't understand why Internals and InternalsController need to be separate. > > This is because > > - Internals follows Document lifetime and holds document, This is not actually true. Internals is exposed to JS, so it has its own lifetime. It has an unnecessary inheritance from FrameDestructionObserver, but it doesn't depend on frames or documents in reality. > that enable us to kill redudant Document* parameters from many Internals APIs > in following changes. This needs to be done, but is not a reason to keep Internals and InternalsController separate. > InternalsController needs to survive across document to keep InternalSettings > alive. So it need to follow Page lifetime. I think that you are really saying that InternalSettings need to match Page lifetime, and that I'd agree with. We already have two objects (Internals and InternalSettings), and son't need a third one. > - Supplement<T> cannot be RefCounted. But Internals should be. If Supplement cannot do something, don't use it :). Relying on some other code is not an excuse for wrong design.
Hi Alexey, thanks for your feedback! (In reply to comment #10) > > > I don't understand why Internals and InternalsController need to be separate. > > > > This is because > > > > - Internals follows Document lifetime and holds document, > > This is not actually true. Internals is exposed to JS, so it has its own lifetime. It has an unnecessary inheritance from FrameDestructionObserver, but it doesn't depend on frames or documents in reality. Yeah, I'm going to get rid of FrameDestructionObserver from Internals. > > > that enable us to kill redudant Document* parameters from many Internals APIs > > in following changes. > > This needs to be done, but is not a reason to keep Internals and InternalsController separate. > Makes sense. > > InternalsController needs to survive across document to keep InternalSettings > > alive. So it need to follow Page lifetime. > > I think that you are really saying that InternalSettings need to match Page lifetime, and that I'd agree with. We already have two objects (Internals and InternalSettings), and son't need a third one. > Yup, On lifetime, that what I meant to say. > > - Supplement<T> cannot be RefCounted. But Internals should be. > > If Supplement cannot do something, don't use it :). Relying on some other code is not an excuse for wrong design. Agreed. I'll try to figure out another approach. Canceling r?.
Created attachment 147491 [details] Patch
Comment on attachment 147491 [details] Patch Attachment 147491 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12961075
Comment on attachment 147491 [details] Patch Attachment 147491 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12957216
Comment on attachment 147491 [details] Patch Attachment 147491 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12953449
Comment on attachment 147491 [details] Patch Attachment 147491 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12954307
Created attachment 147501 [details] Patch
Created attachment 147690 [details] Patch
Comment on attachment 147690 [details] Patch Attachment 147690 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12956506
Created attachment 147698 [details] Patch
Comment on attachment 147698 [details] Patch Attachment 147698 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12956585
Comment on attachment 147698 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147698&action=review I didn't examine the patch carefully. The direction looks good. > Source/WebCore/ChangeLog:15 > + Now InternalSettings object is created per frame instead of per page. I thought the opposite was the goal. Is this accurate? > Source/WebCore/ChangeLog:20 > + No new tests. covered by existing ones. I'd have just omitted this line. Changes in functionality that's not exposed to customers rarely need tests. > Source/WebCore/testing/Internals.idl:86 > - void setPagination(in Document document, in DOMString mode, in long gap) raises(DOMException); > + //void setPagination(in Document document, in DOMString mode, in long gap) raises(DOMException); ?
Comment on attachment 147698 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147698&action=review >> Source/WebCore/ChangeLog:15 >> + Now InternalSettings object is created per frame instead of per page. > > I thought the opposite was the goal. Is this accurate? I didn't get it as well, sorry. Btw, Internals will still be bounded to ScriptExecutionContext, right? > Source/WebKit2/ChangeLog:10 > +2012-06-13 Tim Horton <timothy_horton@apple.com> This was probably added by mistake, wasn't it? > Source/WebCore/testing/Internals.idl:118 > + //void setUserPreferredLanguages(in sequence<String> languages); Both of the above were moved to InternalSettings, right? You forgot to remove these lines, I believe :). > Source/WebCore/testing/Internals.idl:142 > + //void allowRoundingHacks(); Ditto.
Comment on attachment 147698 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147698&action=review >>> Source/WebCore/ChangeLog:15 >>> + Now InternalSettings object is created per frame instead of per page. >> >> I thought the opposite was the goal. Is this accurate? > > I didn't get it as well, sorry. Btw, Internals will still be bounded to ScriptExecutionContext, right? - I changed a design and makes InternalSettings more like just a wrapper of WebCore::Settings, and extract could-be-changed states into InternalSettings::Backup. Then, per-test state is encapsulated behind the Backup object. - The lifetime of Internals object is ScriptExceptionContext, as Jesus mentioned. It's going to be a simple wrapper of Document objecct. Frame object is no longer involved its lifetime. >> Source/WebCore/ChangeLog:20 >> + No new tests. covered by existing ones. > > I'd have just omitted this line. Changes in functionality that's not exposed to customers rarely need tests. True. will remove. >> Source/WebKit2/ChangeLog:10 >> +2012-06-13 Tim Horton <timothy_horton@apple.com> > > This was probably added by mistake, wasn't it? Oops. Thanks for the catch! Will fix.
I'll update the patch once my gardening duty is over. Thanks for your feedback!
Created attachment 148733 [details] Patch
> > I thought the opposite was the goal. Is this accurate? Alexey is right. My description was wrong. Fixed it. Sorry for the confusion. I'd appreciate if you folks took another look. Thanks,
Comment on attachment 148733 [details] Patch Attachment 148733 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13030016 New failing tests: media/track/track-language-preference.html
Created attachment 148738 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 148751 [details] Patch
Comment on attachment 148751 [details] Patch Attachment 148751 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13036090 New failing tests: media/track/track-language-preference.html
Created attachment 148777 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 148953 [details] Patch
(In reply to comment #33) > Created an attachment (id=148953) [details] > Patch LGTM!
(In reply to comment #34) > > LGTM! Thanks! I'd like to hear Alexey's impression. Or find someone who can stamp this a few days later.
Looks like a good improvement to me. - internals.allowRoundingHacks(); + internals.settings.allowRoundingHacks(); This specifically I'm not so sure about. Better internal objects don't mean that callers should be using a longer expression just to change a setting. The same lifetime improvements could be made without changing how one invokes settings setters.
Created attachment 149656 [details] Patch
Hi Alexey, thanks for your feedback! (In reply to comment #36) > Looks like a good improvement to me. > > - internals.allowRoundingHacks(); > + internals.settings.allowRoundingHacks(); > > This specifically I'm not so sure about. Better internal objects don't mean that callers should be using a longer expression just to change a setting. The same lifetime improvements could be made without changing how one invokes settings setters. Makes sense. I updated the patch to leave original functions as aliases. Although we no longer need to pass Document as a parameter, such cleanup will come as followup changes.
ap: ping?
Comment on attachment 149656 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149656&action=review I won't have a chance to review carefully soon, but this has been extensively discussed, and looks like a definitive improvement. > Source/WebCore/testing/Internals.idl:-135 > - boolean hasAutocorrectedMarker(in Document document, in long from, in long length) raises (DOMException); This change is not mentioned in ChangeLog comments. Was this function unused?
Comment on attachment 149656 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149656&action=review > Source/WebCore/ChangeLog:8 > + This change Unfinished sentence here.
Comment on attachment 149656 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149656&action=review Thanks! I'll land this shortly. >> Source/WebCore/testing/Internals.idl:-135 >> - boolean hasAutocorrectedMarker(in Document document, in long from, in long length) raises (DOMException); > > This change is not mentioned in ChangeLog comments. Was this function unused? Looks like an accidental removal. Will recover bofore landing. Thanks for the catch!
Created attachment 151625 [details] Patch for landing
Comment on attachment 151625 [details] Patch for landing Rejecting attachment 151625 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: git/webkit-commit-queue/Source/WebKit/chromium/tools/grit --revision 55 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 21>At revision 55. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/13180372
Created attachment 151646 [details] Patch for landing
Comment on attachment 151646 [details] Patch for landing Clearing flags on attachment: 151646 Committed r122326: <http://trac.webkit.org/changeset/122326>
All reviewed patches have been landed. Closing bug.
*** Bug 89610 has been marked as a duplicate of this bug. ***
*** Bug 87993 has been marked as a duplicate of this bug. ***