Persist browseraction properties across restarts

Persist browseraction properties across restarts
Persist browser action properties across restarts even if other properties are modified before the old ones are loaded. (Note: only properties which are NOT modified are persisted.)
BUG=348775
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255588

[Devlin, 2014-03-03 23:40:28]

Jeffrey, mind taking a look?  (I feel bad that I've sent my last half-dozen
reviews to Ben...)

[Jeffrey Yasskin, 2014-03-04 22:11:10]

I'm not done yet, but here are some comments.

https://codereview.chromium.org/186013003/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/extension_action/browser_action_browsertest.cc
(right):

https://codereview.chromium.org/186013003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/browser_action_browsertest.cc:37:
content::RunMessageLoop();
Use MessageLoopRunner when you need a Quit closure:
https://code.google.com/p/chromium/codesearch/#chromium/src/content/public/te...

https://codereview.chromium.org/186013003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/browser_action_browsertest.cc:50:
ASSERT_TRUE(extension);
Assert that the extension's name is kExtensionName in this test, so it's clear
that they match.

https://codereview.chromium.org/186013003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/browser_action_browsertest.cc:78:
// Wait for the StateStore to load, and fetch the defaults.
Are you guaranteed that the StateStore has _not_ loaded before this call? Can
you assert that? This would make sure that if the behavior ever breaks, the test
will fail consistently instead of becoming flaky.

https://codereview.chromium.org/186013003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/186013003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:149:
!action->HasDefaultPopupUrl()) {
I've gotten lost in the multiple-negatives. You have if (!HasDefault), which is
equivalent to if (HasBeenModified), but the comment says "don't set if has been
modified", which would make me expect the opposite condition.

Oh, HasBeenModified isn't the opposite of HasDefault. Default is just one of the
aspects of this key. I think you shouldn't add HasDefault*: you should call
action->HasPopupUrl(kTabId)

[Jeffrey Yasskin, 2014-03-04 22:37:28]

Ok, got through everything.

https://codereview.chromium.org/186013003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/186013003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:141:
const int kTabId = ExtensionAction::kDefaultTabId;
Arguably, this should be named 'kDefaultTabId'.

[Devlin, 2014-03-04 22:38:40]

two quick questions

https://codereview.chromium.org/186013003/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/extension_action/browser_action_browsertest.cc
(right):

https://codereview.chromium.org/186013003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/browser_action_browsertest.cc:37:
content::RunMessageLoop();
On 2014/03/04 22:11:11, Jeffrey Yasskin wrote:
> Use MessageLoopRunner when you need a Quit closure:
>
https://code.google.com/p/chromium/codesearch/#chromium/src/content/public/te...

I don't know that I can... The callback expected by
StateStore::GetExtensionValue() must take a parameter of a value - is there a
way to pass in the QuitClosure, even though it takes no params?

https://codereview.chromium.org/186013003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/186013003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:149:
!action->HasDefaultPopupUrl()) {
On 2014/03/04 22:11:11, Jeffrey Yasskin wrote:
> I've gotten lost in the multiple-negatives. You have if (!HasDefault), which
is
> equivalent to if (HasBeenModified), but the comment says "don't set if has
been
> modified", which would make me expect the opposite condition.
> 
> Oh, HasBeenModified isn't the opposite of HasDefault. Default is just one of
the
> aspects of this key. I think you shouldn't add HasDefault*: you should call
> action->HasPopupUrl(kTabId)

Note that there is _only_ HasPopupUrl, not Has*.  Should I then add Has* (i.e.
s/HasDefault/Has/)?

[Jeffrey Yasskin, 2014-03-04 22:49:19]

https://codereview.chromium.org/186013003/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/extension_action/browser_action_browsertest.cc
(right):

https://codereview.chromium.org/186013003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/browser_action_browsertest.cc:37:
content::RunMessageLoop();
On 2014/03/04 22:38:40, D Cronin wrote:
> On 2014/03/04 22:11:11, Jeffrey Yasskin wrote:
> > Use MessageLoopRunner when you need a Quit closure:
> >
>
https://code.google.com/p/chromium/codesearch/#chromium/src/content/public/te...
> 
> I don't know that I can... The callback expected by
> StateStore::GetExtensionValue() must take a parameter of a value - is there a
> way to pass in the QuitClosure, even though it takes no params?

Oh, oops. We have an IgnoreResult, but no IgnoreArguments. You should still use
a MessageLoopRunner or RunLoop instead of a direct call to
MessageLoopForUI::current()->Quit(), but your helper function does look
necessary.

https://codereview.chromium.org/186013003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/186013003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:149:
!action->HasDefaultPopupUrl()) {
On 2014/03/04 22:38:40, D Cronin wrote:
> On 2014/03/04 22:11:11, Jeffrey Yasskin wrote:
> > I've gotten lost in the multiple-negatives. You have if (!HasDefault), which
> is
> > equivalent to if (HasBeenModified), but the comment says "don't set if has
> been
> > modified", which would make me expect the opposite condition.
> > 
> > Oh, HasBeenModified isn't the opposite of HasDefault. Default is just one of
> the
> > aspects of this key. I think you shouldn't add HasDefault*: you should call
> > action->HasPopupUrl(kTabId)
> 
> Note that there is _only_ HasPopupUrl, not Has*.  Should I then add Has* (i.e.
> s/HasDefault/Has/)?

I think that'll be more readable, so yes please.

[Devlin, 2014-03-05 18:27:17]

https://codereview.chromium.org/186013003/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/extension_action/browser_action_browsertest.cc
(right):

https://codereview.chromium.org/186013003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/browser_action_browsertest.cc:37:
content::RunMessageLoop();
On 2014/03/04 22:49:20, Jeffrey Yasskin wrote:
> On 2014/03/04 22:38:40, D Cronin wrote:
> > On 2014/03/04 22:11:11, Jeffrey Yasskin wrote:
> > > Use MessageLoopRunner when you need a Quit closure:
> > >
> >
>
https://code.google.com/p/chromium/codesearch/#chromium/src/content/public/te...
> > 
> > I don't know that I can... The callback expected by
> > StateStore::GetExtensionValue() must take a parameter of a value - is there
a
> > way to pass in the QuitClosure, even though it takes no params?
> 
> Oh, oops. We have an IgnoreResult, but no IgnoreArguments. You should still
use
> a MessageLoopRunner or RunLoop instead of a direct call to
> MessageLoopForUI::current()->Quit(), but your helper function does look
> necessary.

Done.

https://codereview.chromium.org/186013003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/browser_action_browsertest.cc:50:
ASSERT_TRUE(extension);
On 2014/03/04 22:11:11, Jeffrey Yasskin wrote:
> Assert that the extension's name is kExtensionName in this test, so it's clear
> that they match.

Done.

https://codereview.chromium.org/186013003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/browser_action_browsertest.cc:78:
// Wait for the StateStore to load, and fetch the defaults.
On 2014/03/04 22:11:11, Jeffrey Yasskin wrote:
> Are you guaranteed that the StateStore has _not_ loaded before this call? Can
> you assert that? This would make sure that if the behavior ever breaks, the
test
> will fail consistently instead of becoming flaky.

Per our discussion offline, there is no good way of ensuring that the StateStore
has not loaded by this point. But, there is a 1-second delay in the StateStore
which will _almost_ guarantee that it will not have. Instead, we'll insert a
warning, and use that to be able to trace back if the test is flaky.  Also added
a comment so that the log doesn't get taken out in the latest "Kill all the
logs" spree.

https://codereview.chromium.org/186013003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/186013003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:141:
const int kTabId = ExtensionAction::kDefaultTabId;
On 2014/03/04 22:37:28, Jeffrey Yasskin wrote:
> Arguably, this should be named 'kDefaultTabId'.

Done.

https://codereview.chromium.org/186013003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:149:
!action->HasDefaultPopupUrl()) {
On 2014/03/04 22:49:20, Jeffrey Yasskin wrote:
> On 2014/03/04 22:38:40, D Cronin wrote:
> > On 2014/03/04 22:11:11, Jeffrey Yasskin wrote:
> > > I've gotten lost in the multiple-negatives. You have if (!HasDefault),
which
> > is
> > > equivalent to if (HasBeenModified), but the comment says "don't set if has
> > been
> > > modified", which would make me expect the opposite condition.
> > > 
> > > Oh, HasBeenModified isn't the opposite of HasDefault. Default is just one
of
> > the
> > > aspects of this key. I think you shouldn't add HasDefault*: you should
call
> > > action->HasPopupUrl(kTabId)
> > 
> > Note that there is _only_ HasPopupUrl, not Has*.  Should I then add Has*
(i.e.
> > s/HasDefault/Has/)?
> 
> I think that'll be more readable, so yes please.

Done.

[Jeffrey Yasskin, 2014-03-05 20:23:33]

lgtm once the Has*() functions take a tab_id.

https://codereview.chromium.org/186013003/diff/200001/chrome/browser/extensio...
File
chrome/browser/extensions/api/extension_action/browser_action_browsertest.cc
(right):

https://codereview.chromium.org/186013003/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/extension_action/browser_action_browsertest.cc:84:
// This is *not* a noisy or usless log. If the state store has already been
sp: usless

It'll probably be more effective to say up front that "If this log becomes
frequent, this test is losing its effectiveness, and we need to find a more
invasive way of making sure the test's StateStore initializes after extensions
gets their onStartup event."

https://codereview.chromium.org/186013003/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/186013003/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:149:
!action->HasPopupUrl()) {
I think this needs to be HasPopupUrl(kDefaultTabId) or it's not obvious which
tab is getting queried.

https://codereview.chromium.org/186013003/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/extension_action.h (right):

https://codereview.chromium.org/186013003/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/extension_action.h:184: bool HasPopupUrl() const;
These need to take a tab_id, like the Get* functions, and return whether that
tab has the specified attribute.

[Devlin, 2014-03-05 23:04:31]

https://codereview.chromium.org/186013003/diff/200001/chrome/browser/extensio...
File
chrome/browser/extensions/api/extension_action/browser_action_browsertest.cc
(right):

https://codereview.chromium.org/186013003/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/extension_action/browser_action_browsertest.cc:84:
// This is *not* a noisy or usless log. If the state store has already been
On 2014/03/05 20:23:33, Jeffrey Yasskin wrote:
> sp: usless
> 
> It'll probably be more effective to say up front that "If this log becomes
> frequent, this test is losing its effectiveness, and we need to find a more
> invasive way of making sure the test's StateStore initializes after extensions
> gets their onStartup event."

Done.

https://codereview.chromium.org/186013003/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/186013003/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:149:
!action->HasPopupUrl()) {
On 2014/03/05 20:23:33, Jeffrey Yasskin wrote:
> I think this needs to be HasPopupUrl(kDefaultTabId) or it's not obvious which
> tab is getting queried.

Done.

https://codereview.chromium.org/186013003/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/extension_action.h (right):

https://codereview.chromium.org/186013003/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/extension_action.h:184: bool HasPopupUrl() const;
On 2014/03/05 20:23:33, Jeffrey Yasskin wrote:
> These need to take a tab_id, like the Get* functions, and return whether that
> tab has the specified attribute.

Whoops, misunderstood earlier. Done.

[Devlin, 2014-03-05 23:04:40]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-03-05 23:06:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/186013003/...

[commit-bot: I haz the power, 2014-03-05 23:33:43]

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

[commit-bot: I haz the power, 2014-03-05 23:33:44]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Devlin, 2014-03-05 23:37:10]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-03-05 23:39:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/186013003/...

[commit-bot: I haz the power, 2014-03-06 00:18:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/186013003/...

[commit-bot: I haz the power, 2014-03-06 01:36:57]

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

[commit-bot: I haz the power, 2014-03-06 01:36:57]

Try jobs failed on following builders: mac_chromium_rel

[Devlin, 2014-03-06 17:01:39]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-03-06 17:03:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/186013003/...

[commit-bot: I haz the power, 2014-03-06 18:16:09]

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

[commit-bot: I haz the power, 2014-03-06 18:16:09]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Devlin, 2014-03-06 19:17:24]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-03-06 19:20:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/186013003/...

[commit-bot: I haz the power, 2014-03-06 19:47:11]

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

[commit-bot: I haz the power, 2014-03-06 19:47:12]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests,
check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, events_unittests, gfx_unittests,
google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests,
jingle_unittests, media_unittests, mini_installer_test, nacl_integration,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Devlin, 2014-03-06 20:45:01]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-03-06 21:00:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/186013003/...

[commit-bot: I haz the power, 2014-03-06 21:29:14]

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

[commit-bot: I haz the power, 2014-03-06 21:29:15]

Try jobs failed on following builders: mac_chromium_compile_dbg

[Devlin, 2014-03-06 21:36:53]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-03-06 21:54:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/186013003/...

[commit-bot: I haz the power, 2014-03-06 23:12:01]

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

[commit-bot: I haz the power, 2014-03-06 23:12:01]

Try jobs failed on following builders: mac_chromium_compile_dbg

[Devlin, 2014-03-06 23:17:43]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-03-06 23:21:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/186013003/...

[commit-bot: I haz the power, 2014-03-07 11:44:43]

Change committed as 255588

[xhwang, 2014-03-07 18:00:34]

On 2014/03/07 11:44:43, I haz the power (commit-bot) wrote:
> Change committed as 255588

Reverted in http://src.chromium.org/viewvc/chrome?revision=255655&view=revision
because of test failures. Sorry about that.