Calling isSecureContext() with no arguments

Calling isSecureContext() with no arguments

isSecureContext() can now be called without an error message string argument. Also removed unused strings/error messages previously used in calls to isSecureContext().

Note: This affects all variants of ExecutionContext

BUG=567298
R=tkent@chromium.org

Committed: https://crrev.com/071f91ee902c1f5bfab3c2495ba3605d2733e8e8
Cr-Commit-Position: refs/heads/master@{#366331}

[tkent, 2015-12-09 01:10:58]

tkent@chromium.org changed reviewers:
+ estark@chromium.org

[tkent, 2015-12-09 01:10:59]

+estark@

[estark, 2015-12-09 01:46:20]

estark@chromium.org changed reviewers:
+ mkwst@chromium.org

[estark, 2015-12-09 01:46:20]

+mkwst

itsbriany: Thanks for doing this! So are you planning to do a follow-up CL that
adds a zero-argument version of isPotentiallyTrustworthy()? i.e. avoids the
overhead of the string copy when it won't be used?

Also, can you please format the CL description like so?
Subject:
<short, less than 72 char description>
Description:
<repeat subject as first line>

<longer description here>

https://codereview.chromium.org/1504403003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ExecutionContext.h (right):

https://codereview.chromium.org/1504403003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ExecutionContext.h:159: virtual bool
isSecureContext(const SecureContextCheck = StandardSecureContextCheck) const =
0;
Could you instead provide the implementation in ExecutionContext.cpp, instead of
repeating it in each subclass?

[itsbriany, 2015-12-09 02:10:53]

Description was changed from

==========
isSecureContext() can now be called without an error message string argument.
Also removed unused strings/error messages previously used in calls to
isSecureContext().

Note: This affects all variants of ExecutionContext

BUG=567298
R=tkent@chromium.org
==========

to

==========
Calling isSecureContext() with no arguments

isSecureContext() can now be called without an error message string argument.
Also removed unused strings/error messages previously used in calls to
isSecureContext().

Note: This affects all variants of ExecutionContext

BUG=567298
R=tkent@chromium.org
==========

[itsbriany, 2015-12-09 02:15:20]

On 2015/12/09 01:46:20, estark wrote:
> +mkwst
> 
> itsbriany: Thanks for doing this! So are you planning to do a follow-up CL
that
> adds a zero-argument version of isPotentiallyTrustworthy()? i.e. avoids the
> overhead of the string copy when it won't be used?
> 
> Also, can you please format the CL description like so?
> Subject:
> <short, less than 72 char description>
> Description:
> <repeat subject as first line>
> 
> <longer description here>
> 
>
https://codereview.chromium.org/1504403003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/ExecutionContext.h (right):
> 
>
https://codereview.chromium.org/1504403003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/ExecutionContext.h:159: virtual bool
> isSecureContext(const SecureContextCheck = StandardSecureContextCheck) const =
> 0;
> Could you instead provide the implementation in ExecutionContext.cpp, instead
of
> repeating it in each subclass?

Sure, I'll overload isPotentiallyTrustworthy() to avoid using empty strings in
the followup CL.

I will also implement the overload of isSecureContext() with no arguments in
ExecutionContext to avoid redundant code. 
Thanks for catching that!

[Mike West, 2015-12-10 08:58:06]

On 2015/12/09 at 02:15:20, itsBrianY wrote:
> Sure, I'll overload isPotentiallyTrustworthy() to avoid using empty strings in
the followup CL.
> 
> I will also implement the overload of isSecureContext() with no arguments in
ExecutionContext to avoid redundant code. 
> Thanks for catching that!

(I'll defer to Emily for the review, her suggestions make tons of sense. Happy
to stamp it once she's happy.)

[Mike West, 2015-12-10 08:59:35]

On 2015/12/10 at 08:58:06, Mike West wrote:
> On 2015/12/09 at 02:15:20, itsBrianY wrote:
> > Sure, I'll overload isPotentiallyTrustworthy() to avoid using empty strings
in the followup CL.
> > 
> > I will also implement the overload of isSecureContext() with no arguments in
ExecutionContext to avoid redundant code. 
> > Thanks for catching that!
> 
> (I'll defer to Emily for the review, her suggestions make tons of sense. Happy
to stamp it once she's happy.)

Also, thank you for signing the CLA. I've verified that we can accept code from
you. :)

[itsbriany, 2015-12-11 23:07:12]

The CQ bit was checked by itsbriany@gmail.com

[itsbriany, 2015-12-11 23:07:13]

The CQ bit was unchecked by itsbriany@gmail.com

[estark, 2015-12-14 20:50:44]

estark@chromium.org changed reviewers:
+ itsbriany@gmail.com
- itsBrianY@gmail.com

[estark, 2015-12-14 20:50:44]

Sorry it took me a while to get back to you on this!

https://codereview.chromium.org/1504403003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/1504403003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:370: bool
SecurityOrigin::isPotentiallyTrustworthy() const
Hmmm... since you've already introduced the no-argument version of
isPotentiallyTrustworthy(), would you mind actually using it from
Document::isSecureContext() in this CL instead of deferring that to a follow-up
CL?

If you don't mind doing that, I think you can leave the zero-argument definition
of ExecutionContext::isSecureContext() as you have it, except make it virtual,
and then override it in Document. That would (I think) allow you to remove the
"ExecutionContext::" qualifiers from the callsites, which are kind of ugly and
would be nice to avoid if possible.

Reusing the Document::isSecureContext(errorMessage) code for a zero-argument
version is going to be a little ugly, but maybe you can do something like this?

// helper function
bool isOriginPotentiallyTrustworthy(SecurityOrigin* origin, String*
errorMessage) {
  if (errorMessage)
    return origin->isPotentiallyTrustworthy(*errorMessage);
  else
    return origin->isPotentiallyTrustworthy();
}

bool Document::isSecureContextImpl(String* errorMessage, const
SecureContextCheck privilegeContextCheck) const {
  // Current code of isSecureContext here, replacing
  // calls to origin->isPotentiallyTrustworthy with
  // isOriginPotentiallyTrustworhy
}

bool Document::isSecureContext(String& errorMessage, const SecureContextCheck
privilegeContextCheck) const {
  return isSecureContextImpl(&errorMessage, privilegeContextCheck);
}

bool Document::isSecureContex(const SecureContextCheck privilegeContextCheck)
const {
  return isSecureContextImpl(nullptr, privilegeContextCheck);
}

If you'd rather do it in a follow-up CL, though, I think that's fine too.

https://codereview.chromium.org/1504403003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:371: {
I would suggest the following:

bool SecurityOrigin::isPotentiallyTrustworthy(String& errorMessage) const {
    if (isPotentiallyTrustworthy())
        return true;
    errorMessage = "...";
    return false;
}

bool SecurityOrigin::isPotentiallyTrustworthy() const {
    // put the logic that's currently on lines 359-367 here
}

That way we don't do the string copy when the error message isn't going to be
used.

[itsbriany, 2015-12-16 16:51:28]

On 2015/12/14 20:50:44, estark wrote:
> Sorry it took me a while to get back to you on this!
> 
>
https://codereview.chromium.org/1504403003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp (right):
> 
>
https://codereview.chromium.org/1504403003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:370: bool
> SecurityOrigin::isPotentiallyTrustworthy() const
> Hmmm... since you've already introduced the no-argument version of
> isPotentiallyTrustworthy(), would you mind actually using it from
> Document::isSecureContext() in this CL instead of deferring that to a
follow-up
> CL?
> 
> If you don't mind doing that, I think you can leave the zero-argument
definition
> of ExecutionContext::isSecureContext() as you have it, except make it virtual,
> and then override it in Document. That would (I think) allow you to remove the
> "ExecutionContext::" qualifiers from the callsites, which are kind of ugly and
> would be nice to avoid if possible.
> 
> Reusing the Document::isSecureContext(errorMessage) code for a zero-argument
> version is going to be a little ugly, but maybe you can do something like
this?
> 
> // helper function
> bool isOriginPotentiallyTrustworthy(SecurityOrigin* origin, String*
> errorMessage) {
>   if (errorMessage)
>     return origin->isPotentiallyTrustworthy(*errorMessage);
>   else
>     return origin->isPotentiallyTrustworthy();
> }
> 
> bool Document::isSecureContextImpl(String* errorMessage, const
> SecureContextCheck privilegeContextCheck) const {
>   // Current code of isSecureContext here, replacing
>   // calls to origin->isPotentiallyTrustworthy with
>   // isOriginPotentiallyTrustworhy
> }
> 
> bool Document::isSecureContext(String& errorMessage, const SecureContextCheck
> privilegeContextCheck) const {
>   return isSecureContextImpl(&errorMessage, privilegeContextCheck);
> }
> 
> bool Document::isSecureContex(const SecureContextCheck privilegeContextCheck)
> const {
>   return isSecureContextImpl(nullptr, privilegeContextCheck);
> }
> 
> If you'd rather do it in a follow-up CL, though, I think that's fine too.
> 
>
https://codereview.chromium.org/1504403003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:371: {
> I would suggest the following:
> 
> bool SecurityOrigin::isPotentiallyTrustworthy(String& errorMessage) const {
>     if (isPotentiallyTrustworthy())
>         return true;
>     errorMessage = "...";
>     return false;
> }
> 
> bool SecurityOrigin::isPotentiallyTrustworthy() const {
>     // put the logic that's currently on lines 359-367 here
> }
> 
> That way we don't do the string copy when the error message isn't going to be
> used.

@estark
I just refactored isSecureContext(), adding the changes you suggested.

However, I was wondering how I could make my changes on the same CL (currently I
only know how to do followup CLs).
Also, what is the best way to test my new code to ensure that I have not broken
anything? Would I run trybots, or are there some specific Blink unit tests that
I can run to test the new code from isSecureContext()?

I also noticed how long it takes to build this code when I make changes to
header files (especially Document.h). I assume that in a practical dev
environment, people have multiple builds  of the code base?

If you have more suggestions to improve this code, I am happy to add them. :)

[estark, 2015-12-18 00:00:58]

Thanks, this is looking great! Just a couple more small comments inline, and
some answers to your questions below.

> However, I was wondering how I could make my changes on the same CL (currently
I
> only know how to do followup CLs).
> Also, what is the best way to test my new code to ensure that I have not
broken
> anything? Would I run trybots, or are there some specific Blink unit tests
that
> I can run to test the new code from isSecureContext()?

I think we have a terminology mix-up. :) What you've been doing so far is adding
new patchsets to the same CL. (https://codereview.chromium.org/1504403003 is the
CL, and it has multiple patchsets. When you land it, the latest patchset will
get committed as a single commit in the revision history.) Sometimes people will
request changes in follow-up CLs, which would mean you would land this change
(using the Commit checkbox below the latest patchset), then start a new branch
and run 'git cl upload' to create a new CL: that would be a new
codereview.chromium.org/<issue #>. Does that make sense?

> Also, what is the best way to test my new code to ensure that I have not
broken
> anything? Would I run trybots, or are there some specific Blink unit tests
that
> I can run to test the new code from isSecureContext()?

I'm not sure if you have trybot access; I'll start some so we can see if the
tests are passing. If you keep contributing to Chromium, then we can request
trybot access so that you'll be able to run trybots yourself.

If you want to run some relevant tests locally, for this change you probably
want to run some of the layout tests. There are instructions here:
https://dev.chromium.org/developers/testing/webkit-layout-tests

It looks like the tests in LayoutTests/http/tests/security/secureContexts and
LayoutTests/http/tests/security/powerfulFeatureRestrictions probably the most
relevant for this change.

> I also noticed how long it takes to build this code when I make changes to
> header files (especially Document.h). I assume that in a practical dev
> environment, people have multiple builds  of the code base?

How long does it take? It should be doing incremental builds which shouldn't
take as long as a fresh build, but unfortunately it can still take quite a long
time. Many Chrome developers just have very beefy machines to deal with this.

https://codereview.chromium.org/1504403003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1504403003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:3306: bool
Document::isOriginPotentiallyTrustworthy(SecurityOrigin* origin, String*
errorMessage) const
Since this doesn't need to access any methods or data members on Document, it
could just be a helper function at the top of the file (somewhere around line
343). It doesn't need to be a Document method.

https://codereview.chromium.org/1504403003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/1504403003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:203: if
(document()->ExecutionContext::isSecureContext()) {
You should remove the "ExecutionContext::" qualifier now, as you want to run the
Document override, not the ExecutionContext version.

https://codereview.chromium.org/1504403003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/1504403003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/DOMWindow.cpp:166: return
document()->ExecutionContext::isSecureContext(ExecutionContext::StandardSecureContextCheck);
same here

https://codereview.chromium.org/1504403003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/forms/FileInputType.cpp (right):

https://codereview.chromium.org/1504403003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/FileInputType.cpp:246: if
(document->ExecutionContext::isSecureContext())
same

[estark, 2015-12-18 00:01:13]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-18 00:04:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1504403003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1504403003/40001

[commit-bot: I haz the power, 2015-12-18 01:35:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-18 01:35:54]

Dry run: This issue passed the CQ dry run.

[itsbriany, 2015-12-18 18:09:37]

The CQ bit was checked by itsbriany@gmail.com

[commit-bot: I haz the power, 2015-12-18 18:10:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1504403003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1504403003/60001

[commit-bot: I haz the power, 2015-12-18 18:10:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-18 18:10:05]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[itsbriany, 2015-12-18 18:14:54]

On 2015/12/18 00:00:58, estark wrote:
> Thanks, this is looking great! Just a couple more small comments inline, and
> some answers to your questions below.
> 
> > However, I was wondering how I could make my changes on the same CL
(currently
> I
> > only know how to do followup CLs).
> > Also, what is the best way to test my new code to ensure that I have not
> broken
> > anything? Would I run trybots, or are there some specific Blink unit tests
> that
> > I can run to test the new code from isSecureContext()?
> 
> I think we have a terminology mix-up. :) What you've been doing so far is
adding
> new patchsets to the same CL. (https://codereview.chromium.org/1504403003 is
the
> CL, and it has multiple patchsets. When you land it, the latest patchset will
> get committed as a single commit in the revision history.) Sometimes people
will
> request changes in follow-up CLs, which would mean you would land this change
> (using the Commit checkbox below the latest patchset), then start a new branch
> and run 'git cl upload' to create a new CL: that would be a new
> codereview.chromium.org/<issue #>. Does that make sense?
> 
> > Also, what is the best way to test my new code to ensure that I have not
> broken
> > anything? Would I run trybots, or are there some specific Blink unit tests
> that
> > I can run to test the new code from isSecureContext()?
> 
> I'm not sure if you have trybot access; I'll start some so we can see if the
> tests are passing. If you keep contributing to Chromium, then we can request
> trybot access so that you'll be able to run trybots yourself.
> 
> If you want to run some relevant tests locally, for this change you probably
> want to run some of the layout tests. There are instructions here:
> https://dev.chromium.org/developers/testing/webkit-layout-tests
> 
> It looks like the tests in LayoutTests/http/tests/security/secureContexts and
> LayoutTests/http/tests/security/powerfulFeatureRestrictions probably the most
> relevant for this change.
> 
> > I also noticed how long it takes to build this code when I make changes to
> > header files (especially Document.h). I assume that in a practical dev
> > environment, people have multiple builds  of the code base?
> 
> How long does it take? It should be doing incremental builds which shouldn't
> take as long as a fresh build, but unfortunately it can still take quite a
long
> time. Many Chrome developers just have very beefy machines to deal with this.
> 
>
https://codereview.chromium.org/1504403003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/1504403003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Document.cpp:3306: bool
> Document::isOriginPotentiallyTrustworthy(SecurityOrigin* origin, String*
> errorMessage) const
> Since this doesn't need to access any methods or data members on Document, it
> could just be a helper function at the top of the file (somewhere around line
> 343). It doesn't need to be a Document method.
> 
>
https://codereview.chromium.org/1504403003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right):
> 
>
https://codereview.chromium.org/1504403003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Fullscreen.cpp:203: if
> (document()->ExecutionContext::isSecureContext()) {
> You should remove the "ExecutionContext::" qualifier now, as you want to run
the
> Document override, not the ExecutionContext version.
> 
>
https://codereview.chromium.org/1504403003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/DOMWindow.cpp (right):
> 
>
https://codereview.chromium.org/1504403003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/DOMWindow.cpp:166: return
>
document()->ExecutionContext::isSecureContext(ExecutionContext::StandardSecureContextCheck);
> same here
> 
>
https://codereview.chromium.org/1504403003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/forms/FileInputType.cpp (right):
> 
>
https://codereview.chromium.org/1504403003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/forms/FileInputType.cpp:246: if
> (document->ExecutionContext::isSecureContext())
> same

Thanks, that really makes things clear!
The code is now refactored so that Document::isSecureContext() is called instead
of ExecutionContext::isSecureContext().

[estark, 2015-12-20 20:04:40]

Great, thanks! lgtm but you'll need approval from an OWNER (likely mkwst) before
you can land this.

mkwst: can you take a look when you're back in the office?

[itsbriany, 2015-12-21 03:28:03]

The CQ bit was checked by itsbriany@gmail.com

[commit-bot: I haz the power, 2015-12-21 03:28:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1504403003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1504403003/60001

[commit-bot: I haz the power, 2015-12-21 03:36:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-21 03:36:53]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tkent, 2015-12-21 03:41:18]

rs lgtm

[itsbriany, 2015-12-21 04:12:09]

The CQ bit was checked by itsbriany@gmail.com

[commit-bot: I haz the power, 2015-12-21 04:12:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1504403003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1504403003/60001

[commit-bot: I haz the power, 2015-12-21 05:33:56]

Description was changed from

==========
Calling isSecureContext() with no arguments

isSecureContext() can now be called without an error message string argument.
Also removed unused strings/error messages previously used in calls to
isSecureContext().

Note: This affects all variants of ExecutionContext

BUG=567298
R=tkent@chromium.org
==========

to

==========
Calling isSecureContext() with no arguments

isSecureContext() can now be called without an error message string argument.
Also removed unused strings/error messages previously used in calls to
isSecureContext().

Note: This affects all variants of ExecutionContext

BUG=567298
R=tkent@chromium.org
==========

[commit-bot: I haz the power, 2015-12-21 05:33:57]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-21 05:35:01]

Description was changed from

==========
Calling isSecureContext() with no arguments

isSecureContext() can now be called without an error message string argument.
Also removed unused strings/error messages previously used in calls to
isSecureContext().

Note: This affects all variants of ExecutionContext

BUG=567298
R=tkent@chromium.org
==========

to

==========
Calling isSecureContext() with no arguments

isSecureContext() can now be called without an error message string argument.
Also removed unused strings/error messages previously used in calls to
isSecureContext().

Note: This affects all variants of ExecutionContext

BUG=567298
R=tkent@chromium.org

Committed: https://crrev.com/071f91ee902c1f5bfab3c2495ba3605d2733e8e8
Cr-Commit-Position: refs/heads/master@{#366331}
==========

[commit-bot: I haz the power, 2015-12-21 05:35:02]

Patchset 4 (id:??) landed as
https://crrev.com/071f91ee902c1f5bfab3c2495ba3605d2733e8e8
Cr-Commit-Position: refs/heads/master@{#366331}