Bug 41529 - Add the ability for user scripts and user styles to affect just the top frame
Summary: Add the ability for user scripts and user styles to affect just the top frame
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-02 12:27 PDT by Aaron Boodman
Modified: 2010-07-08 21:14 PDT (History)
4 users (show)

See Also:


Attachments
Patch (52.50 KB, patch)
2010-07-02 12:37 PDT, Aaron Boodman
no flags Details | Formatted Diff | Diff
Patch (54.92 KB, patch)
2010-07-02 14:39 PDT, Aaron Boodman
no flags Details | Formatted Diff | Diff
Patch (41.45 KB, patch)
2010-07-07 14:00 PDT, Aaron Boodman
no flags Details | Formatted Diff | Diff
Patch (51.26 KB, patch)
2010-07-07 15:11 PDT, Aaron Boodman
timothy: review+
timothy: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aaron Boodman 2010-07-02 12:27:58 PDT
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.
Comment 1 Aaron Boodman 2010-07-02 12:37:53 PDT
Created attachment 60390 [details]
Patch
Comment 2 Darin Fisher (:fishd, Google) 2010-07-02 13:26:41 PDT
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.
Comment 3 Aaron Boodman 2010-07-02 14:39:43 PDT
Created attachment 60405 [details]
Patch
Comment 4 Aaron Boodman 2010-07-02 14:40:35 PDT
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?
Comment 5 WebKit Review Bot 2010-07-02 21:07:08 PDT
Attachment 60405 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3343760
Comment 6 Darin Fisher (:fishd, Google) 2010-07-02 22:18:49 PDT
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.
Comment 7 Aaron Boodman 2010-07-07 10:50:35 PDT
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.
Comment 8 Aaron Boodman 2010-07-07 11:57:31 PDT
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.
Comment 9 Timothy Hatcher 2010-07-07 12:31:49 PDT
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.
Comment 10 Aaron Boodman 2010-07-07 14:00:12 PDT
Created attachment 60774 [details]
Patch
Comment 11 Aaron Boodman 2010-07-07 14:06:23 PDT
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.
Comment 12 Aaron Boodman 2010-07-07 15:11:55 PDT
Created attachment 60786 [details]
Patch
Comment 13 Aaron Boodman 2010-07-07 15:12:29 PDT
Ok, all comments addressed. Darin, Timothy, please take another look.
Comment 14 Timothy Hatcher 2010-07-08 10:36:24 PDT
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.
Comment 15 Aaron Boodman 2010-07-08 18:15:27 PDT
Committed r62876: <http://trac.webkit.org/changeset/62876>
Comment 16 WebKit Review Bot 2010-07-08 18:20:45 PDT
http://trac.webkit.org/changeset/62876 might have broken Qt Linux Release minimal, Qt Linux ARMv5 Release, and Qt Linux ARMv7 Release
Comment 17 Adam Barth 2010-07-08 21:14:08 PDT
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