Bug 63026 – Wire in checkIfRunInsecureContent to the chromium WebKit API

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Chris Evans]

As per $SUMMARY

[Chris Evans]

Created attachment 97887 [details]
Patch

[WebKit Review Bot]

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

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]

Created attachment 97892 [details]
Patch

Move function definitions to somewhere slightly more suitable.

[WebKit Review Bot]

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

(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 on attachment 97892 [details]
Patch

R=me w/ style fixes.

[Chris Evans]

Created attachment 98044 [details]
Patch

Fix style nits for commit.

[Chris Evans]

Created attachment 98046 [details]
Patch

Put reviewer into ChangeLog; using commit-queue.

[WebKit Review Bot]

Comment on attachment 98046 [details]
Patch

Clearing flags on attachment: 98046

Committed r89376: <http://trac.webkit.org/changeset/89376>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

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

(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! :-)