Summary: | Add the ability for user scripts and user styles to affect just the top frame | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Aaron Boodman <aa> | ||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, eric, hyatt, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Attachments: |
|
Description
Aaron Boodman
2010-07-02 12:27:58 PDT
Created attachment 60390 [details]
Patch
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.
Created attachment 60405 [details]
Patch
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? Attachment 60405 [details] did not build on win: Build output: http://webkit-commit-queue.appspot.com/results/3343760 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.
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. 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 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.
Created attachment 60774 [details]
Patch
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. Created attachment 60786 [details]
Patch
Ok, all comments addressed. Darin, Timothy, please take another look. 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. Committed r62876: <http://trac.webkit.org/changeset/62876> http://trac.webkit.org/changeset/62876 might have broken Qt Linux Release minimal, Qt Linux ARMv5 Release, and Qt Linux ARMv7 Release 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 |