Calling isSecureContext() with no arguments

third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp

Index: third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp
diff --git a/third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp b/third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp
index 4d75bab3529a61a2a191458c4ae88b59fa55fdeb..70847664d1c2898dc88947a04c172204998ca981 100644
--- a/third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp
+++ b/third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp
@@ -367,6 +367,12 @@ bool SecurityOrigin::isPotentiallyTrustworthy(String& errorMessage) const
     return false;
 }
 
+bool SecurityOrigin::isPotentiallyTrustworthy() const

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

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.

+{

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

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.

+    String unusedErrorMessage;
+    return isPotentiallyTrustworthy(unusedErrorMessage);
+}
+
 void SecurityOrigin::grantLoadLocalResources()
 {
     // Granting privileges to some, but not all, documents in a SecurityOrigin