RESOLVED FIXED 63026
Wire in checkIfRunInsecureContent to the chromium WebKit API
https://bugs.webkit.org/show_bug.cgi?id=63026
Summary Wire in checkIfRunInsecureContent to the chromium WebKit API
Chris Evans
Reported 2011-06-20 16:35:05 PDT
As per $SUMMARY
Attachments
Patch (2.60 KB, patch)
2011-06-20 16:37 PDT, Chris Evans
no flags
Patch (2.58 KB, patch)
2011-06-20 17:11 PDT, Chris Evans
fishd: review+
fishd: commit-queue-
Patch (2.57 KB, patch)
2011-06-21 12:46 PDT, Chris Evans
no flags
Patch (2.56 KB, patch)
2011-06-21 12:52 PDT, Chris Evans
no flags
Chris Evans
Comment 1 2011-06-20 16:37:50 PDT
WebKit Review Bot
Comment 2 2011-06-20 16:40:40 PDT
Attachment 97887 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1 Source/WebKit/chromium/public/WebFrame.h:633: The parameter name "url" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/src/WebFrameImpl.h:225: The parameter name "url" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Darin Fisher (:fishd, Google)
Comment 3 2011-06-20 16:43:55 PDT
Comment on attachment 97887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97887&action=review > Source/WebKit/chromium/public/WebFrame.h:633 > + virtual bool checkIfRunInsecureContent(const WebURL& url) const = 0; nit: you might want to find a better section of WebFrame.h to put this in. You've appended to the section that has methods used for testing. nit: the name of this function is a mouthful. it is generally better to use names that are real phrases. i'm lacking for advice. i suspect it is nice to make this method symmetric with didRunInsecureContent, which probably explains the current name. still, it feels like a tongue twister :-)
Chris Evans
Comment 4 2011-06-20 16:54:25 PDT
I'll move the function to a better place and re-upload. "checkIfRunInsecureContent" is actually symmetric with the public function in WebCore::FrameLoader, is that ok?
Chris Evans
Comment 5 2011-06-20 17:11:19 PDT
Created attachment 97892 [details] Patch Move function definitions to somewhere slightly more suitable.
WebKit Review Bot
Comment 6 2011-06-20 17:14:05 PDT
Attachment 97892 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1 Source/WebKit/chromium/public/WebFrame.h:277: The parameter name "url" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/src/WebFrameImpl.h:118: The parameter name "url" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Darin Fisher (:fishd, Google)
Comment 7 2011-06-21 10:49:53 PDT
(In reply to comment #4) > I'll move the function to a better place and re-upload. > "checkIfRunInsecureContent" is actually symmetric with the public function in WebCore::FrameLoader, is that ok? I think that one is questionably named too. I still don't have much of a better suggestion. checkIfWouldBeInsecureToRunContent(url) /sigh/
Darin Fisher (:fishd, Google)
Comment 8 2011-06-21 10:50:42 PDT
Comment on attachment 97892 [details] Patch R=me w/ style fixes.
Chris Evans
Comment 9 2011-06-21 12:46:11 PDT
Created attachment 98044 [details] Patch Fix style nits for commit.
Chris Evans
Comment 10 2011-06-21 12:52:39 PDT
Created attachment 98046 [details] Patch Put reviewer into ChangeLog; using commit-queue.
WebKit Review Bot
Comment 11 2011-06-21 13:54:52 PDT
Comment on attachment 98046 [details] Patch Clearing flags on attachment: 98046 Committed r89376: <http://trac.webkit.org/changeset/89376>
WebKit Review Bot
Comment 12 2011-06-21 13:54:57 PDT
All reviewed patches have been landed. Closing bug.
Adam Barth
Comment 13 2011-06-21 14:01:35 PDT
> nit: you might want to find a better section of WebFrame.h to put this in. You've > appended to the section that has methods used for testing. This patch represents a dangerous pattern of dumping APIs onto WebFrame. If left unchecked, this design pressure will cause WebFrame to grow into a monster class, like WebCore::Frame used to be. We're planning to move the mixed-content related functions off of FrameLoader for this reason and unto an object called MixedContentChecker (or something snazzier). I'm not sure what the best thing to do for the Chromium WebKit API is in this case, but it's easy for this stuff to accumulate on 3-line context diff at a time.
Darin Fisher (:fishd, Google)
Comment 14 2011-06-21 15:50:13 PDT
(In reply to comment #13) > > nit: you might want to find a better section of WebFrame.h to put this in. You've > > appended to the section that has methods used for testing. > > This patch represents a dangerous pattern of dumping APIs onto WebFrame. If left unchecked, this design pressure will cause WebFrame to grow into a monster class, like WebCore::Frame used to be. > > We're planning to move the mixed-content related functions off of FrameLoader for this reason and unto an object called MixedContentChecker (or something snazzier). > > I'm not sure what the best thing to do for the Chromium WebKit API is in this case, but it's easy for this stuff to accumulate on 3-line context diff at a time. Absolutely agreed. This is why there is some effort to section off WebFrame, grouping like methods, etc. John has also been cleaving off bits and pieces of these interfaces in accordance with better refactoring and modularity on the Chromium side. I support doing more of that! :-)
Note You need to log in before you can comment on or make changes to this bug.