Calling isSecureContext() with no arguments

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

 /*
  * Copyright (C) 2007 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1.  Redistributions of source code must retain the above copyright
  *     notice, this list of conditions and the following disclaimer.
  * 2.  Redistributions in binary form must reproduce the above copyright
@@ skipping 349 matching lines @@
     if (SchemeRegistry::shouldTreatURLSchemeAsSecure(m_protocol) || isLocal() || isLocalhost())
         return true;
 
     if (SecurityPolicy::isOriginWhiteListedTrustworthy(*this))
         return true;
 
     errorMessage = "Only secure origins are allowed (see: https://goo.gl/Y0ZkNV).";
     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
     // is a security hazard because the documents without the privilege can
     // obtain the privilege by injecting script into the documents that have
     // been granted the privilege.
     m_canLoadLocalResources = true;
 }
 
 void SecurityOrigin::grantUniversalAccess()
@@ skipping 161 matching lines @@
 }
 
 void SecurityOrigin::transferPrivilegesFrom(PassOwnPtr<PrivilegeData> privilegeData)
 {
     m_universalAccess = privilegeData->m_universalAccess;
     m_canLoadLocalResources = privilegeData->m_canLoadLocalResources;
     m_blockLocalAccessFromLocalOrigin = privilegeData->m_blockLocalAccessFromLocalOrigin;
 }
 
 } // namespace blink