Bug 88499 – WebCoreSupport needs objects each of which follows major WebCore objects

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Hajime Morrita]

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.

[Hajime Morrita]

Created attachment 146208 [details]
Patch

[Hajime Morrita]

This change is a part of reorg for addressing Bug 87933.

[Hajime Morrita]

Created attachment 146215 [details]
Patch

[Build Bot]

Comment on attachment 146215 [details]
Patch

Attachment 146215 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12913158

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Alexey Proskuryakov]

Comment on attachment 146215 [details]
Patch

I don't understand why Internals and InternalsController need to be separate.

[Hajime Morrita]

(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.

[Jesus Sanchez-Palencia]

(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.

[Alexey Proskuryakov]

> > 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.

[Hajime Morrita]

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?.

[Hajime Morrita]

Created attachment 147491 [details]
Patch

[Early Warning System Bot]

Comment on attachment 147491 [details]
Patch

Attachment 147491 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12961075

[Build Bot]

Comment on attachment 147491 [details]
Patch

Attachment 147491 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12957216

[Build Bot]

Comment on attachment 147491 [details]
Patch

Attachment 147491 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12953449

[Gyuyoung Kim]

Comment on attachment 147491 [details]
Patch

Attachment 147491 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12954307

[Hajime Morrita]

Created attachment 147501 [details]
Patch

[Hajime Morrita]

Created attachment 147690 [details]
Patch

[Build Bot]

Comment on attachment 147690 [details]
Patch

Attachment 147690 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12956506

[Hajime Morrita]

Created attachment 147698 [details]
Patch

[WebKit Review Bot]

Comment on attachment 147698 [details]
Patch

Attachment 147698 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12956585

[Alexey Proskuryakov]

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);

?

[Jesus Sanchez-Palencia]

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
> [phone]  Tim Horton  <[e-mail]>

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.

[Hajime Morrita]

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
>> [phone]  Tim Horton  <[e-mail]>
> 
> This was probably added by mistake, wasn't it?

Oops. Thanks for the catch! Will fix.

[Hajime Morrita]

I'll update the patch once my gardening duty is over. 
Thanks for your feedback!

[Hajime Morrita]

Created attachment 148733 [details]
Patch

[Hajime Morrita]

> > 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,

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Hajime Morrita]

Created attachment 148751 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Hajime Morrita]

Created attachment 148953 [details]
Patch

[Jesus Sanchez-Palencia]

(In reply to comment #33)
> Created an attachment (id=148953) [details]
> Patch

LGTM!

[Hajime Morrita]

(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.

[Alexey Proskuryakov]

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.

[Hajime Morrita]

Created attachment 149656 [details]
Patch

[Hajime Morrita]

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.

[Hajime Morrita]

ap: ping?

[Alexey Proskuryakov]

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?

[Alexey Proskuryakov]

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.

[Hajime Morrita]

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!

[Hajime Morrita]

Created attachment 151625 [details]
Patch for landing

[WebKit Review Bot]

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

[Hajime Morrita]

Created attachment 151646 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 151646 [details]
Patch for landing

Clearing flags on attachment: 151646

Committed r122326: <http://trac.webkit.org/changeset/122326>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

*** Bug 89610 has been marked as a duplicate of this bug. ***

[Tony Chang]

*** Bug 87993 has been marked as a duplicate of this bug. ***