Fix for ERROR:url_pattern_set.cc(240)] Invalid url pattern: chrome://print/*

Fix for ERROR:url_pattern_set.cc(240)] Invalid url pattern: chrome://print/*

This issue is due to usage of UserScript::ValidUserScriptSchemes()
instead of UserScript::kValidUserScriptSchemes,

we will add CHROMEUI scheme in valid schemes mask if extension is component extension.

BUG=467230
Committed: https://crrev.com/75277b0f5f91981603f04c7d39a582b700e45830
Cr-Commit-Position: refs/heads/master@{#325197}

[Deepak, 2015-03-20 15:59:01]

deepak.m1@samsung.com changed reviewers:
+ rockot@chromium.org

[Deepak, 2015-03-20 15:59:02]

PTAL at a small change for passing proper value of valid_schemes as
UserScript::kValidUserScriptSchemes for resolving this console error.
Thanks

[Ken Rockot(use gerrit already), 2015-03-20 16:12:04]

On 2015/03/20 15:59:02, Deepak wrote:
> PTAL at a small change for passing proper value of valid_schemes as
> UserScript::kValidUserScriptSchemes for resolving this console error.
> Thanks

NOT LGTM

First: You linked a bug which is calling for a reduction in log message noise.
While technically this might reduce log message volume, it does so by changing
behavior in a significant way.

Second: I'm fairly certain we DO NOT want user scripts running in chrome scheme
contexts. Can you point me to a discussion or bug which suggests otherwise?

[Deepak, 2015-03-20 16:19:55]

On 2015/03/20 16:12:04, Ken Rockot wrote:
> On 2015/03/20 15:59:02, Deepak wrote:
> > PTAL at a small change for passing proper value of valid_schemes as
> > UserScript::kValidUserScriptSchemes for resolving this console error.
> > Thanks
> 
> NOT LGTM
> 
> First: You linked a bug which is calling for a reduction in log message noise.
> While technically this might reduce log message volume, it does so by changing
> behavior in a significant way.
> 
> Second: I'm fairly certain we DO NOT want user scripts running in chrome
scheme
> contexts. Can you point me to a discussion or bug which suggests otherwise?

In my debugging I found that
This issue is due to value of valid_schemes comes 15.
That is coming from 
ReadPrefAsURLPatternSet(
      extension_id, JoinPrefs(pref_key, kPrefScriptableHosts),
      &scriptable_hosts, UserScript::ValidUserScriptSchemes());
As
base::CommandLine::ForCurrentProcess() does not have 
switches::kExtensionsOnChromeURLs  switch.

And UserScript::kValidUserScriptSchemes has been changed to
UserScript::ValidUserScriptSchemes().

I understand that it might change significant way.

Can we just change LOG to DLOG to satisfy console ?

please correct me if I misunderstood.

[Ken Rockot(use gerrit already), 2015-03-20 16:23:50]

On 2015/03/20 16:19:55, Deepak wrote:
> On 2015/03/20 16:12:04, Ken Rockot wrote:
> > On 2015/03/20 15:59:02, Deepak wrote:
> > > PTAL at a small change for passing proper value of valid_schemes as
> > > UserScript::kValidUserScriptSchemes for resolving this console error.
> > > Thanks
> > 
> > NOT LGTM
> > 
> > First: You linked a bug which is calling for a reduction in log message
noise.
> > While technically this might reduce log message volume, it does so by
changing
> > behavior in a significant way.
> > 
> > Second: I'm fairly certain we DO NOT want user scripts running in chrome
> scheme
> > contexts. Can you point me to a discussion or bug which suggests otherwise?
> 
> In my debugging I found that
> This issue is due to value of valid_schemes comes 15.
> That is coming from 
> ReadPrefAsURLPatternSet(
>       extension_id, JoinPrefs(pref_key, kPrefScriptableHosts),
>       &scriptable_hosts, UserScript::ValidUserScriptSchemes());
> As
> base::CommandLine::ForCurrentProcess() does not have 
> switches::kExtensionsOnChromeURLs  switch.
> 
> And UserScript::kValidUserScriptSchemes has been changed to
> UserScript::ValidUserScriptSchemes().
> 
> I understand that it might change significant way.
> 
> Can we just change LOG to DLOG to satisfy console ?
> 
> please correct me if I misunderstood.

Your understanding is correct, but so is the existing behavior (in most cases). 
Looking into this further, it seems to be caused by the component extension for
OOP PDF support. See also bug 444752.

I think a proper fix here would be to make an exception for component extensions
only. You could modify the ExtensionPrefs behavior so that it still calls
ValidUserScriptSchemes, but that it also adds CHROMEUI to the valid schemes mask
if the extension is a component extension.

[Ken Rockot(use gerrit already), 2015-03-20 16:24:48]

rockot@chromium.org changed reviewers:
+ kalman@chromium.org

[Ken Rockot(use gerrit already), 2015-03-20 16:24:49]

+kalman to sanity-check my suggestion

[not at google - send to devlin, 2015-03-20 16:36:45]

I agree, not lgtm. To iterate on Ken's suggestion, how about passing the
extension into ValidUserScriptSchemes and determining it from that?

It looks like ValidUserScriptSchemes already sort-of does this by some very
wrong looking code - uses an optional argument meaning "no really trust me".
Needs to be fixed. Passing an Extension* in is much safer.

[Deepak, 2015-03-21 09:45:09]

@Rockot
Thanks for review and suggestion.
Changes done for adding CHROMEUI in valid schemes when extension is component
extension.
PTAL
Thanks

[Deepak, 2015-03-24 10:17:12]

@Rockot
Thanks for review and suggestion.
Changes done for adding CHROMEUI in valid schemes when extension is component
extension.
PTAL
Thanks

[Ken Rockot(use gerrit already), 2015-03-24 16:49:09]

LGTM, thanks!

Unfortunately I think kalman will need to also give his LGTM before this can
land. He will be back in a week.

https://codereview.chromium.org/1025613003/diff/20001/extensions/browser/exte...
File extensions/browser/extension_prefs.cc (right):

https://codereview.chromium.org/1025613003/diff/20001/extensions/browser/exte...
extensions/browser/extension_prefs.cc:606: Manifest::COMPONENT ==
static_cast<Manifest::Location>(location_value)) {
nit: Please swap the operand order here. i.e.:

static_cast<Manifest::Location>(location_value) == Manifest::COMPONENT

This is convention in chromium code.

[Deepak, 2015-03-25 03:33:56]

On 2015/03/24 16:49:09, Ken Rockot wrote:
> LGTM, thanks!
> 
> Unfortunately I think kalman will need to also give his LGTM before this can
> land. He will be back in a week.
> 
>
https://codereview.chromium.org/1025613003/diff/20001/extensions/browser/exte...
> File extensions/browser/extension_prefs.cc (right):
> 
>
https://codereview.chromium.org/1025613003/diff/20001/extensions/browser/exte...
> extensions/browser/extension_prefs.cc:606: Manifest::COMPONENT ==
> static_cast<Manifest::Location>(location_value)) {
> nit: Please swap the operand order here. i.e.:
> 
> static_cast<Manifest::Location>(location_value) == Manifest::COMPONENT
> 
> This is convention in chromium code.

@Rockot
Thanks for review.
Changes done as suggested.
I will wait for kalman's LGTM.

[not at google - send to devlin, 2015-03-30 19:54:09]

https://codereview.chromium.org/1025613003/diff/40001/extensions/browser/exte...
File extensions/browser/extension_prefs.cc (right):

https://codereview.chromium.org/1025613003/diff/40001/extensions/browser/exte...
extensions/browser/extension_prefs.cc:566: const base::DictionaryValue*
extensions = GetExtensionPref(extension_id);
Hm, would be nice to call GetInstalledExtensionInfo in this method instead of
dealing with the pref dict directly. The location casting below makes me uneasy.

https://codereview.chromium.org/1025613003/diff/40001/extensions/browser/exte...
extensions/browser/extension_prefs.cc:604: // extension.
This should probably happen in ReadPrefAsURLPatternSet. It seems unnecessarily
inconsistent that we allow chrome:// in the scriptable but not explicit host
permission set for component extensions.

[Deepak, 2015-04-07 14:25:42]

@kalman
Thanks for review.
Changes done as suggested.
PTAL

https://codereview.chromium.org/1025613003/diff/40001/extensions/browser/exte...
File extensions/browser/extension_prefs.cc (right):

https://codereview.chromium.org/1025613003/diff/40001/extensions/browser/exte...
extensions/browser/extension_prefs.cc:566: const base::DictionaryValue*
extensions = GetExtensionPref(extension_id);
On 2015/03/30 19:54:09, kalman wrote:

Done.

https://codereview.chromium.org/1025613003/diff/40001/extensions/browser/exte...
extensions/browser/extension_prefs.cc:604: // extension.
On 2015/03/30 19:54:09, kalman wrote:

Done.

[not at google - send to devlin, 2015-04-07 20:57:10]

looks fine, but can you write a test?

[Deepak, 2015-04-08 11:18:46]

On 2015/04/07 20:57:10, kalman wrote:
> looks fine, but can you write a test?

@kalman
Thanks for review:
I have tried to write test case for this in extension_prefs_unittest.cc file As:

class TestExtensionPrefsFlags : public ExtensionPrefsTest {
 public:
  ~TestExtensionPrefsFlags() override {}
  void Initialize() override {
      base::DictionaryValue dictionary;
      dictionary.SetString(manifest_keys::kName, "test");
      dictionary.SetString(manifest_keys::kVersion, "0.1");
     scoped_ptr<base::ListValue> content_scripts (new base::ListValue);
      content_scripts->AppendString("chrome://print/*");
      dictionary.Set("active_permissions.scriptable_host",
content_scripts.release());
      webstore_extension_ = prefs_.AddExtensionWithManifestAndFlags(
          dictionary, Manifest::COMPONENT, Extension::NO_FLAGS);
    }
  void Verify() override {
    URLPatternSet scriptable_hosts;
    std::string pref_key = "active_permissions.scriptable_host";
    prefs()->SetAllowFileAccess(webstore_extension_->id(), true);
   
EXPECT_FALSE(prefs()->ReadPrefAsURLPatternSet(webstore_extension_->id(),pref_key,&scriptable_hosts,15));
  }
 private:
    scoped_refptr<Extension> webstore_extension_;
};
TEST_F(TestExtensionPrefsFlags, TestExtensionPrefsFlags) {}

But when I execute this then it is getting returned from ReadPrefAsList() call
from ExtensionPrefs::ReadPrefAsURLPatternSet().
as ext->GetList(pref_key, &out) return false.
That is due to failure of getting list from pref_key.

Can you please suggest if I am missing something for this test case.

Thanks

[not at google - send to devlin, 2015-04-08 17:06:14]

On 2015/04/08 11:18:46, Deepak wrote:
> On 2015/04/07 20:57:10, kalman wrote:
> > looks fine, but can you write a test?
> 
> @kalman
> Thanks for review:
> I have tried to write test case for this in extension_prefs_unittest.cc file
As:
> 
> class TestExtensionPrefsFlags : public ExtensionPrefsTest {
>  public:
>   ~TestExtensionPrefsFlags() override {}
>   void Initialize() override {
>       base::DictionaryValue dictionary;
>       dictionary.SetString(manifest_keys::kName, "test");
>       dictionary.SetString(manifest_keys::kVersion, "0.1");
>      scoped_ptr<base::ListValue> content_scripts (new base::ListValue);
>       content_scripts->AppendString("chrome://print/*");
>       dictionary.Set("active_permissions.scriptable_host",
> content_scripts.release());
>       webstore_extension_ = prefs_.AddExtensionWithManifestAndFlags(
>           dictionary, Manifest::COMPONENT, Extension::NO_FLAGS);
>     }
>   void Verify() override {
>     URLPatternSet scriptable_hosts;
>     std::string pref_key = "active_permissions.scriptable_host";
>     prefs()->SetAllowFileAccess(webstore_extension_->id(), true);
>    
>
EXPECT_FALSE(prefs()->ReadPrefAsURLPatternSet(webstore_extension_->id(),pref_key,&scriptable_hosts,15));
>   }
>  private:
>     scoped_refptr<Extension> webstore_extension_;
> };
> TEST_F(TestExtensionPrefsFlags, TestExtensionPrefsFlags) {}
> 
> But when I execute this then it is getting returned from ReadPrefAsList() call
> from ExtensionPrefs::ReadPrefAsURLPatternSet().
> as ext->GetList(pref_key, &out) return false.
> That is due to failure of getting list from pref_key.
> 
> Can you please suggest if I am missing something for this test case.
> 
> Thanks

Could you upload the test changes you made? I need to see it in context. Having
a whole class for a single test is hopefully not necessary.

[Deepak, 2015-04-09 11:43:40]

Test Case added.
It is working as expected when I am getting extension from GetExtensionPref()
and then checking location.

But when I use GetInstalledExtensionInfo() to get Extensioninfo then test is not
working well, as installed_extension.get() is NULL.
Please suggest how to get installed extension info when we add extension via
test case.
Please correct me if I am missing something.
Thanks

[not at google - send to devlin, 2015-04-09 14:56:07]

https://codereview.chromium.org/1025613003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

https://codereview.chromium.org/1025613003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_prefs_unittest.cc:934: AddPattern(&shosts,
"chrome://print/*");
Thanks for writing this test, getting there, but I think you could do this in a
more principled way. Perhaps the other tests should be more like this too, but,
eh:

webstore_extension_ = ExtensionBuilder()
  .SetManifest(DictionaryBuilder()
    .Set(manifest_keys::kName, "test")
    .Set(manifest_keys::kVersion, "0.1"))
  .SetLocation(Manifest::COMPONENT).Build();

// (btw, a utility in TestExtensionPrefs to add an Extension* would be good),
// it could let you write prefs_->AddExtension(extension.get()).
prefs_->OnExtensionInstalled(extension.get(),
                             Extension::ENABLED,
                             syncer::StringOrdinal::CreateInitialOrdinal(),
                             std::string());

and then in the Verify() method, read a new Extension* back out and assert that
it can access "chrome://print/*".

and I'd feel more comfortable if you tested here that a non-Component extension
*can't* access chrome://print.

and instead of "webstore_extension_" I'd call it just "component_extension_".

https://codereview.chromium.org/1025613003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_prefs_unittest.cc:940: active_perms_.get());
I don't know exactly why that GetInstalledExtensionInfo is failing without
digging into the code. However, I think the new code you have in
extension_prefs.cc now (i.e. deleting what you've commented out) is fine.

https://codereview.chromium.org/1025613003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_prefs_unittest.cc:946:
prefs()->SetAllowFileAccess(webstore_extension_->id(), true);
Why is file access relevant?

https://codereview.chromium.org/1025613003/diff/80001/extensions/browser/exte...
File extensions/browser/extension_prefs.cc (right):

https://codereview.chromium.org/1025613003/diff/80001/extensions/browser/exte...
extensions/browser/extension_prefs.cc:552: const base::DictionaryValue*
extensions = GetExtensionPref(extension_id);
"extension", not "extensions".

https://codereview.chromium.org/1025613003/diff/80001/extensions/browser/exte...
extensions/browser/extension_prefs.cc:555: int location_value;
just "location"?

[Deepak, 2015-04-10 05:16:34]

@kalman
Changes done as suggested and it is working as expected.
PTAL
Thanks

https://codereview.chromium.org/1025613003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

https://codereview.chromium.org/1025613003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_prefs_unittest.cc:934: AddPattern(&shosts,
"chrome://print/*");
On 2015/04/09 14:56:07, kalman wrote:
> Thanks for writing this test, getting there, but I think you could do this in
a
> more principled way. Perhaps the other tests should be more like this too,
but,
> eh:
> 
> webstore_extension_ = ExtensionBuilder()
>   .SetManifest(DictionaryBuilder()
>     .Set(manifest_keys::kName, "test")
>     .Set(manifest_keys::kVersion, "0.1"))
>   .SetLocation(Manifest::COMPONENT).Build();
> 
> // (btw, a utility in TestExtensionPrefs to add an Extension* would be good),
> // it could let you write prefs_->AddExtension(extension.get()).
> prefs_->OnExtensionInstalled(extension.get(),
>                              Extension::ENABLED,
>                              syncer::StringOrdinal::CreateInitialOrdinal(),
>                              std::string());
> 
> and then in the Verify() method, read a new Extension* back out and assert
that
> it can access "chrome://print/*".
> 
> and I'd feel more comfortable if you tested here that a non-Component
extension
> *can't* access chrome://print.
> 
> and instead of "webstore_extension_" I'd call it just "component_extension_".

1) I have added a new function in test_extension_prefs.cc so that we can use
existing infrastructure with Manifest::COMPONENT location.
2) Checking "chrome://print/*" in Verify().
3) Code for When we have non-Component extension then we will get false as it
can not access "chrome://print/*".
4) other nit's addressed.

https://codereview.chromium.org/1025613003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_prefs_unittest.cc:940: active_perms_.get());
On 2015/04/09 14:56:07, kalman wrote:
> I don't know exactly why that GetInstalledExtensionInfo is failing without
> digging into the code. However, I think the new code you have in
> extension_prefs.cc now (i.e. deleting what you've commented out) is fine.

Done.

https://codereview.chromium.org/1025613003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_prefs_unittest.cc:946:
prefs()->SetAllowFileAccess(webstore_extension_->id(), true);
On 2015/04/09 14:56:07, kalman wrote:
> Why is file access relevant?

Done.

https://codereview.chromium.org/1025613003/diff/80001/extensions/browser/exte...
File extensions/browser/extension_prefs.cc (right):

https://codereview.chromium.org/1025613003/diff/80001/extensions/browser/exte...
extensions/browser/extension_prefs.cc:552: const base::DictionaryValue*
extensions = GetExtensionPref(extension_id);
On 2015/04/09 14:56:07, kalman wrote:
> "extension", not "extensions".

Done.

https://codereview.chromium.org/1025613003/diff/80001/extensions/browser/exte...
extensions/browser/extension_prefs.cc:555: int location_value;
On 2015/04/09 14:56:07, kalman wrote:
> just "location"?

Done.

[not at google - send to devlin, 2015-04-10 17:49:26]

https://codereview.chromium.org/1025613003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

https://codereview.chromium.org/1025613003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_prefs_unittest.cc:921:
prefs_.AddExtensionWithManifestLocation("test", Manifest::COMPONENT);
I don't understand. This isn't what I asked you to do.

[Deepak, 2015-04-11 05:35:27]

Changes done as suggested.
PTAL Thanks.

https://codereview.chromium.org/1025613003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

https://codereview.chromium.org/1025613003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_prefs_unittest.cc:921:
prefs_.AddExtensionWithManifestLocation("test", Manifest::COMPONENT);
On 2015/04/10 17:49:26, kalman wrote:
> I don't understand. This isn't what I asked you to do.

Done.

[Deepak, 2015-04-13 15:16:08]

@kalman
PTAL
Thanks

[not at google - send to devlin, 2015-04-13 17:24:35]

https://codereview.chromium.org/1025613003/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

https://codereview.chromium.org/1025613003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/extension_prefs_unittest.cc:931: // Adding a non
component extension.
It would be more consistent to set this up in the same way that
|component_extension_| is set up.

https://codereview.chromium.org/1025613003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/extension_prefs_unittest.cc:957:
EXPECT_FALSE(component_permissions->scriptable_hosts().is_empty());
You should be able to EXPECT_EQ(1u, ....size());

https://codereview.chromium.org/1025613003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/extension_prefs_unittest.cc:969:
component_extension_->id(), pref_key, &scriptable_hosts, 15));
What is 15? Can you alias it to something meaningful?

[Deepak, 2015-04-14 05:05:42]

Thanks for review.
Changes done as suggested.
PTAL
Thanks

https://codereview.chromium.org/1025613003/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

https://codereview.chromium.org/1025613003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/extension_prefs_unittest.cc:931: // Adding a non
component extension.
On 2015/04/13 17:24:35, kalman wrote:
> It would be more consistent to set this up in the same way that
> |component_extension_| is set up.

Done.

https://codereview.chromium.org/1025613003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/extension_prefs_unittest.cc:957:
EXPECT_FALSE(component_permissions->scriptable_hosts().is_empty());
On 2015/04/13 17:24:35, kalman wrote:
> You should be able to EXPECT_EQ(1u, ....size());

Done.

https://codereview.chromium.org/1025613003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/extension_prefs_unittest.cc:969:
component_extension_->id(), pref_key, &scriptable_hosts, 15));
On 2015/04/13 17:24:35, kalman wrote:
> What is 15? Can you alias it to something meaningful?

Done.

[not at google - send to devlin, 2015-04-14 15:38:35]

lgtm

https://codereview.chromium.org/1025613003/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

https://codereview.chromium.org/1025613003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_prefs_unittest.cc:971:
EXPECT_TRUE(no_component_permissions->scriptable_hosts().is_empty());
Btw I actually prefer (but often forget myself) to use EXPECT_EQ(0u, ...size())
for these, so that you get better failure messages (e.g. "expected 0 but got 1"
or "expected 0 got 2" is better than "failed !is_empty").

[Deepak, 2015-04-15 04:41:43]

On 2015/04/14 15:38:35, kalman wrote:
> lgtm
> 
>
https://codereview.chromium.org/1025613003/diff/140001/chrome/browser/extensi...
> File chrome/browser/extensions/extension_prefs_unittest.cc (right):
> 
>
https://codereview.chromium.org/1025613003/diff/140001/chrome/browser/extensi...
> chrome/browser/extensions/extension_prefs_unittest.cc:971:
> EXPECT_TRUE(no_component_permissions->scriptable_hosts().is_empty());
> Btw I actually prefer (but often forget myself) to use EXPECT_EQ(0u,
...size())
> for these, so that you get better failure messages (e.g. "expected 0 but got
1"
> or "expected 0 got 2" is better than "failed !is_empty").

Thanks for review.
Changes done as suggested.
Thanks.

[Deepak, 2015-04-15 04:41:49]

The CQ bit was checked by deepak.m1@samsung.com

[Deepak, 2015-04-15 04:41:49]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1025613003/#ps160001
(title: "Changes as per review comments.")

[commit-bot: I haz the power, 2015-04-15 04:41:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1025613003/160001

[Deepak, 2015-04-15 04:42:41]

Thanks for review.
Changes done as suggested.
Thanks

https://codereview.chromium.org/1025613003/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

https://codereview.chromium.org/1025613003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_prefs_unittest.cc:971:
EXPECT_TRUE(no_component_permissions->scriptable_hosts().is_empty());
On 2015/04/14 15:38:35, kalman wrote:
> Btw I actually prefer (but often forget myself) to use EXPECT_EQ(0u,
...size())
> for these, so that you get better failure messages (e.g. "expected 0 but got
1"
> or "expected 0 got 2" is better than "failed !is_empty").

Done.

[commit-bot: I haz the power, 2015-04-15 06:30:27]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-04-15 06:31:13]

Patchset 9 (id:??) landed as
https://crrev.com/75277b0f5f91981603f04c7d39a582b700e45830
Cr-Commit-Position: refs/heads/master@{#325197}