Issue 2902103002: MD Bookmarks: Disable 'Open in Incognito Window' when Incognito is disabled

tsergeant

Description was changed from ========== MD Bookmarks: Disable 'Open in Incognito Window' when Incognito is ...

3 years, 7 months ago (2017-05-24 06:31:47 UTC) #1

tsergeant

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

3 years, 6 months ago (2017-05-26 05:45:46 UTC) #4

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2902103002/60001

3 years, 6 months ago (2017-05-26 05:46:35 UTC) #5

tsergeant

tsergeant@chromium.org changed reviewers: + calamity@chromium.org

3 years, 6 months ago (2017-05-26 05:47:56 UTC) #6

tsergeant

Here we go! A lot of code to support a pretty small feature, but it ...

3 years, 6 months ago (2017-05-26 05:47:57 UTC) #7

commit-bot: I haz the power

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

3 years, 6 months ago (2017-05-26 07:16:41 UTC) #8

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 6 months ago (2017-05-26 07:16:42 UTC) #9

calamity

https://codereview.chromium.org/2902103002/diff/60001/chrome/browser/resources/md_bookmarks/command_manager.js File chrome/browser/resources/md_bookmarks/command_manager.js (right): https://codereview.chromium.org/2902103002/diff/60001/chrome/browser/resources/md_bookmarks/command_manager.js#newcode92 chrome/browser/resources/md_bookmarks/command_manager.js:92: this.menuIds_ = new Set(); Does this functionally change anything, ...

3 years, 6 months ago (2017-05-29 06:52:28 UTC) #10

tsergeant

https://codereview.chromium.org/2902103002/diff/60001/chrome/browser/resources/md_bookmarks/command_manager.js File chrome/browser/resources/md_bookmarks/command_manager.js (right): https://codereview.chromium.org/2902103002/diff/60001/chrome/browser/resources/md_bookmarks/command_manager.js#newcode92 chrome/browser/resources/md_bookmarks/command_manager.js:92: this.menuIds_ = new Set(); On 2017/05/29 06:52:27, calamity wrote: ...

3 years, 6 months ago (2017-05-30 00:18:22 UTC) #12

tsergeant

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

3 years, 6 months ago (2017-05-30 00:18:33 UTC) #13

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2902103002/100001

3 years, 6 months ago (2017-05-30 00:18:39 UTC) #14

commit-bot: I haz the power

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

3 years, 6 months ago (2017-05-30 01:35:39 UTC) #15

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 6 months ago (2017-05-30 01:35:39 UTC) #16

tsergeant

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

3 years, 6 months ago (2017-05-30 06:01:04 UTC) #17

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2902103002/120001

3 years, 6 months ago (2017-05-30 06:01:51 UTC) #18

commit-bot: I haz the power

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

3 years, 6 months ago (2017-05-30 07:49:08 UTC) #19

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 6 months ago (2017-05-30 07:49:09 UTC) #20

calamity

https://codereview.chromium.org/2902103002/diff/60001/chrome/browser/resources/md_bookmarks/command_manager.js File chrome/browser/resources/md_bookmarks/command_manager.js (right): https://codereview.chromium.org/2902103002/diff/60001/chrome/browser/resources/md_bookmarks/command_manager.js#newcode92 chrome/browser/resources/md_bookmarks/command_manager.js:92: this.menuIds_ = new Set(); On 2017/05/30 00:18:21, tsergeant wrote: ...

3 years, 6 months ago (2017-06-01 04:07:44 UTC) #21

https://codereview.chromium.org/2902103002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/command_manager.js (right):

https://codereview.chromium.org/2902103002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/command_manager.js:92: this.menuIds_ = new
Set();
On 2017/05/30 00:18:21, tsergeant wrote:
> On 2017/05/29 06:52:27, calamity wrote:
> > Does this functionally change anything, or is it just data cleanup?
> 
> Menu item bindings are based entirely on the menuIds_ set, so clearing it each
> time the menu is closed forces the menu items to update each time the menu is
> opened.

I see. So menu items that have enable conditions that could have changed since
the last right click on the items will update properly?

https://codereview.chromium.org/2902103002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/test_store.js (right):

https://codereview.chromium.org/2902103002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/test_store.js:91: * promise after that, call
`expectAction` again.
On 2017/05/30 00:18:21, tsergeant wrote:
> On 2017/05/29 06:52:28, calamity wrote:
> > Seems a bit risky to have subsequent calls to waitForAction immediately
> resolve.
> > I think it would be better to explicitly clear the map when the promise is
> > resolved so that subsequent calls to waitForAction fail.
> 
> Doing it this way is necessary to support the following situation:
> 
>   expectAction('select')
>   dispatchSelect()
>   waitForAction('select').then(...)
> 
> If the map gets cleared after resolve, this doesn't work when dispatchSelect()
> is synchronous, since the promise would be gone by the time waitForAction is
> called.
> 
> This might work if dispatchSelect is asynchronous, depending on the timing,
but
> there's plenty of situations where the timing is not really dependable. For
> example, in the Policy test, the action is dispatched after the callback to
> chrome.bookmarks.getTree().
> 
> The alternative is to get the promise before there's any chance of the action
> being dispatched:
> 
>   waitForAction('select').then(...)
>   dispatchSelect()
> 
> But this makes tests much harder to read/write.
> 
> So, I think having the promise stick around until it is explicitly cleared
makes
> the tests a bit nicer, and it also roughly matches the API of TestBrowserProxy
> from settings.

Can you work around the synchronicity issue? (e.g would returning
promise.then(<delete from map>) from waitForAction work?)

My concern is that this API will lead to tests that pass when they shouldn't,
because someone cargo culted from an example with a single wait and it wasn't
obvious that another expect was necessary. I'd prefer a guarantee of failure
when the API isn't used as expected.

I mean, I'm happy to stick with this if there's a case where multiple
waitForActions are needed from one expectAction, but I can't really think of
one.

I think TestBrowserProxy would also benefit from such a constraint.

tsergeant

https://codereview.chromium.org/2902103002/diff/60001/chrome/browser/resources/md_bookmarks/command_manager.js File chrome/browser/resources/md_bookmarks/command_manager.js (right): https://codereview.chromium.org/2902103002/diff/60001/chrome/browser/resources/md_bookmarks/command_manager.js#newcode92 chrome/browser/resources/md_bookmarks/command_manager.js:92: this.menuIds_ = new Set(); On 2017/06/01 04:07:44, calamity wrote: ...

3 years, 6 months ago (2017-06-01 05:27:17 UTC) #22

https://codereview.chromium.org/2902103002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/command_manager.js (right):

https://codereview.chromium.org/2902103002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/command_manager.js:92: this.menuIds_ = new
Set();
On 2017/06/01 04:07:44, calamity wrote:
> On 2017/05/30 00:18:21, tsergeant wrote:
> > On 2017/05/29 06:52:27, calamity wrote:
> > > Does this functionally change anything, or is it just data cleanup?
> > 
> > Menu item bindings are based entirely on the menuIds_ set, so clearing it
each
> > time the menu is closed forces the menu items to update each time the menu
is
> > opened.
> 
> I see. So menu items that have enable conditions that could have changed since
> the last right click on the items will update properly?

Yup, exactly.

https://codereview.chromium.org/2902103002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/test_store.js (right):

https://codereview.chromium.org/2902103002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/test_store.js:91: * promise after that, call
`expectAction` again.
On 2017/06/01 04:07:44, calamity wrote:
> On 2017/05/30 00:18:21, tsergeant wrote:
> > On 2017/05/29 06:52:28, calamity wrote:
> > > Seems a bit risky to have subsequent calls to waitForAction immediately
> > resolve.
> > > I think it would be better to explicitly clear the map when the promise is
> > > resolved so that subsequent calls to waitForAction fail.
> > 
> > Doing it this way is necessary to support the following situation:
> > 
> >   expectAction('select')
> >   dispatchSelect()
> >   waitForAction('select').then(...)
> > 
> > If the map gets cleared after resolve, this doesn't work when
dispatchSelect()
> > is synchronous, since the promise would be gone by the time waitForAction is
> > called.
> > 
> > This might work if dispatchSelect is asynchronous, depending on the timing,
> but
> > there's plenty of situations where the timing is not really dependable. For
> > example, in the Policy test, the action is dispatched after the callback to
> > chrome.bookmarks.getTree().
> > 
> > The alternative is to get the promise before there's any chance of the
action
> > being dispatched:
> > 
> >   waitForAction('select').then(...)
> >   dispatchSelect()
> > 
> > But this makes tests much harder to read/write.
> > 
> > So, I think having the promise stick around until it is explicitly cleared
> makes
> > the tests a bit nicer, and it also roughly matches the API of
TestBrowserProxy
> > from settings.
> 
> Can you work around the synchronicity issue? (e.g would returning
> promise.then(<delete from map>) from waitForAction work?)
> 
> My concern is that this API will lead to tests that pass when they shouldn't,
> because someone cargo culted from an example with a single wait and it wasn't
> obvious that another expect was necessary. I'd prefer a guarantee of failure
> when the API isn't used as expected.
> 
> I mean, I'm happy to stick with this if there's a case where multiple
> waitForActions are needed from one expectAction, but I can't really think of
> one.
> 
> I think TestBrowserProxy would also benefit from such a constraint.

Interesting idea to queue up resetting the promise when waitForAction is called.
Done.

tsergeant

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

3 years, 6 months ago (2017-06-01 05:27:26 UTC) #23

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2902103002/140001

3 years, 6 months ago (2017-06-01 05:27:37 UTC) #24

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-01 06:56:36 UTC) #25

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 6 months ago (2017-06-01 06:56:37 UTC) #26

commit-bot: I haz the power

CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2902103002/140001

3 years, 6 months ago (2017-06-05 04:22:26 UTC) #29

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1496636533249460, "parent_rev": "a3ba2766d681b998cde9ca478cac8d3a7bbed218", "commit_rev": "4707d1724a8a2e4db9bc78715fe9b21fb95b10f7"}

3 years, 6 months ago (2017-06-05 05:47:05 UTC) #30

commit-bot: I haz the power

Description was changed from ========== MD Bookmarks: Disable 'Open in Incognito Window' when Incognito is ...

3 years, 6 months ago (2017-06-05 05:47:16 UTC) #31

commit-bot: I haz the power

3 years, 6 months ago (2017-06-05 05:47:18 UTC) #32

Message was sent while issue was closed.

Committed patchset #5 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/4707d1724a8a2e4db9bc78715fe9...

Issue 2902103002: MD Bookmarks: Disable 'Open in Incognito Window' when Incognito is disabled (Closed)

Description

Patch Set 1 #

Patch Set 2 : Add a test #

Patch Set 3 : calamity@ review comments #

Patch Set 4 : Move assert #

Patch Set 5 : Review comment #

Messages