Bug 41529 – Add the ability for user scripts and user styles to affect just the top frame

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Aaron Boodman]

In Chromium, user scripts and styles apply to only the top-level frame by default. This is more performant and typically what the developer wanted. Add the ability for WebKit to specify which behavior a user script should have.

[Aaron Boodman]

Created attachment 60390 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 60390 [details]
Patch

WebKit/chromium/public/WebView.h:298
 +                                           bool runAtStart,
a string of bool parameters is fairly bad for readability.  the better
approach is to define enums.  you should just define enums that match
the WebCore enums.  use AssertMatchingEnums.cpp to help so that you can
just cast between WebCore and WebKit API enums.

[Aaron Boodman]

Created attachment 60405 [details]
Patch

[Aaron Boodman]

Agreed. (In reply to comment #2)
> (From update of attachment 60390 [details])
> WebKit/chromium/public/WebView.h:298
>  +                                           bool runAtStart,
> a string of bool parameters is fairly bad for readability.  the better
> approach is to define enums.  you should just define enums that match
> the WebCore enums.  use AssertMatchingEnums.cpp to help so that you can
> just cast between WebCore and WebKit API enums.

Agreed. Take another look?

[WebKit Review Bot]

Attachment 60405 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3343760

[Darin Fisher (:fishd, Google)]

Comment on attachment 60405 [details]
Patch

WebKit/chromium/public/WebView.h:58
 +      enum WebUserScriptInjectionTime {
The Web-prefix is for things defined at WebKit-namespace scope.  When
declared within a class, you should drop the Web-prefix.

We also have the convention that enum values should be formed like so:

enum Foo {
  FooBar,
  FooBaz
};

I realize that may be a bit verbose for these.

  enum UserScriptInjectionTime {
    UserScriptInjectionTimeAtDocumentStart,
    UserScriptInjectionTimeAtDocumentEnd
  };

  enum UserScriptInjectedFrames {
    UserScriptInjectedFramesAll,
    UserScriptInjectedFramesTopOnly
  };

^^^ That last one is especially awkward.  I'm not sure what would be
better.

[Aaron Boodman]

I will fix the style issues.

Do you mind reviewing the rest of the change? I have pinged dhyatt directly, who originally wrote this code, but I don't think it should be too contentious.

dhyatt: do you want to review this, since you wrote it originally? Otherwise, Darin, can you review the webcore bits? I think this is relatively a minor addition to existing code.

[Aaron Boodman]

Got feedback from dhyatt on irc:

* Can't change webkit api on mac and windows because it will break webkit nightlies. Please remove API modifications and make it preserve current behavior.

[Timothy Hatcher]

Comment on attachment 60405 [details]
Patch

You can't change the WebKit/mac and WebKit/win APIs, since it would cause nightlies to crash if you use Safari 5. You need to add new methods that take the new argument and we can deperacate the old methods until Safari can adopt them.

[Aaron Boodman]

Created attachment 60774 [details]
Patch

[Aaron Boodman]

Hold off, I misread Timothy's comment. I'm going to fix this to add a new method to the mac API so that the test can remain enabled there.

[Aaron Boodman]

Created attachment 60786 [details]
Patch

[Aaron Boodman]

Ok, all comments addressed. Darin, Timothy, please take another look.

[Timothy Hatcher]

Comment on attachment 60786 [details]
Patch

WebCore/dom/Document.cpp:2156
 +              if (sheet->injectedFrames() == InjectInTopFrameOnly && ownerElement())
You should add a test using <frameset>. I'm curious if ownerElement() does the right thing there. Arguably all the frames in a frameset are top level.

WebCore/page/UserStyleSheet.h:42
 +                     UserScriptInjectedFrames injectedFrames)
It is odd to use UserScriptInjectedFrames with stylesheets. Maybe UserContentInjectedFrames? Is there a shared header it could move to as well?

WebKit/mac/WebView/WebView.mm:2382
 +                     injectedFrames:(WebUserScriptInjectedFrames)injectedFrames
It is odd to use WebUserScriptInjectedFrames for the stylesheet case too. Maybe WebUserContentInjectedFrames?

WebKit/mac/WebView/WebViewPrivate.h:512
 +  // FIXME: The following two methods are deprecated in favor of the overloads below that take the WebUserScriptInjectedFrames argument. https://bugs.webkit.org/show_bug.cgi?id=41800.
I don't think the bug is needed.

[Aaron Boodman]

Committed r62876: <http://trac.webkit.org/changeset/62876>

[WebKit Review Bot]

http://trac.webkit.org/changeset/62876 might have broken Qt Linux Release minimal, Qt Linux ARMv5 Release, and Qt Linux ARMv7 Release

[Adam Barth]

Looks like this broke LayoutTests/printing/page-rule-selection.html

http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r62876%20(13247)/printing/page-rule-selection-pretty-diff.html