Fixed dragging a folder from bookmark manager to open all elements in new tabs

Fixed dragging a folder from bookmark manager to open all elements in new tabs

Dropping a bookmark folder on Toolbar or Tab strip will open all of the urls in
separate tabs.

The first URL of dragged/dropped bookmark on toolbar_controller should be opened
in the current opening tab.

BUG=661765
Committed: https://crrev.com/82ba88e6bff66deb3b4a9c06e966637538cd9f89
Cr-Commit-Position: refs/heads/master@{#439713}

[shahriar, 2016-12-18 15:38:21]

Description was changed from

==========
Fixed dragging a folder from bookmark manager to open all elements in new tabs

Dropping a bookmark folder on Omnibox or Tab strip will open all of the urls in
separate tabs.

The first URL of dragged/dropped bookmark on toolbar_controller should be
openned
in the current openning tab.

BUG=661765
==========

to

==========
Fixed dragging a folder from bookmark manager to open all elements in new tabs

Dropping a bookmark folder on Toolbar or Tab strip will open all of the urls in
separate tabs.

The first URL of dragged/dropped bookmark on toolbar_controller should be opened
in the current opening tab.

BUG=661765
==========

[shahriar, 2016-12-18 15:39:06]

shahriar.rostami@gmail.com changed reviewers:
+ avi@chromium.org, erikchen@chromium.org

[shahriar, 2016-12-18 15:42:06]

Hi Erik

I am sorry for being so late on this patch.

I followed the instructions you provide, so the first patch contains all the
previous LGTMed changes, no line added, no line deleted (Bug: 661765), so you
might not need to review it again.

Second patch, contains my fix on bug: 672805.

PTAL.

[Avi (use Gerrit), 2016-12-18 19:20:15]

On 2016/12/18 15:42:06, shahriar wrote:
> Hi Erik
> 
> I am sorry for being so late on this patch.
> 
> I followed the instructions you provide, so the first patch contains all the
> previous LGTMed changes, no line added, no line deleted (Bug: 661765), so you
> might not need to review it again.
> 
> Second patch, contains my fix on bug: 672805.
> 
> PTAL.

That bug was dropping onto the omnibox, which I imagine is the same as dropping
on a tab.

Would this change force the first URL to open in the current tab even for a drop
on a vacant area of the tab strip? That doesn't seem right.

There are three drop locations I can see here: On the omnibox, on a tab, on a
vacant area of the tab strip. There are drops of single items and multiple
items. So we need to do the right thing in at least six cases, so we need to
determine what the right thing to do in those cases are, and then test them.

[shahriar, 2016-12-18 22:31:35]

On 2016/12/18 19:20:15, Avi wrote:
> On 2016/12/18 15:42:06, shahriar wrote:
> > Hi Erik
> > 
> > I am sorry for being so late on this patch.
> > 
> > I followed the instructions you provide, so the first patch contains all the
> > previous LGTMed changes, no line added, no line deleted (Bug: 661765), so
you
> > might not need to review it again.
> > 
> > Second patch, contains my fix on bug: 672805.
> > 
> > PTAL.
> 
> That bug was dropping onto the omnibox, which I imagine is the same as
dropping
> on a tab.
> 
> Would this change force the first URL to open in the current tab even for a
drop
> on a vacant area of the tab strip? That doesn't seem right.
> 
> There are three drop locations I can see here: On the omnibox, on a tab, on a
> vacant area of the tab strip. There are drops of single items and multiple
> items. So we need to do the right thing in at least six cases, so we need to
> determine what the right thing to do in those cases are, and then test them.

avi@ to answer your question, this change does not force the first URL to open
in the current tab
when dropping on a vacant area of tab strip. 

The only case that the current tab would be affected, is that you drop on
toolbar. In that case no matter how
many items are in the bookmark folder, the first bookmark would be opened in the
current tab.

[erikchen, 2016-12-19 17:38:42]

On 2016/12/18 22:31:35, shahriar wrote:
> On 2016/12/18 19:20:15, Avi wrote:
> > On 2016/12/18 15:42:06, shahriar wrote:
> > > Hi Erik
> > > 
> > > I am sorry for being so late on this patch.
> > > 
> > > I followed the instructions you provide, so the first patch contains all
the
> > > previous LGTMed changes, no line added, no line deleted (Bug: 661765), so
> you
> > > might not need to review it again.
> > > 
> > > Second patch, contains my fix on bug: 672805.
> > > 
> > > PTAL.
> > 
> > That bug was dropping onto the omnibox, which I imagine is the same as
> dropping
> > on a tab.
> > 
> > Would this change force the first URL to open in the current tab even for a
> drop
> > on a vacant area of the tab strip? That doesn't seem right.
> > 
> > There are three drop locations I can see here: On the omnibox, on a tab, on
a
> > vacant area of the tab strip. There are drops of single items and multiple
> > items. So we need to do the right thing in at least six cases, so we need to
> > determine what the right thing to do in those cases are, and then test them.
> 
> avi@ to answer your question, this change does not force the first URL to open
> in the current tab
> when dropping on a vacant area of tab strip. 
> 
> The only case that the current tab would be affected, is that you drop on
> toolbar. In that case no matter how
> many items are in the bookmark folder, the first bookmark would be opened in
the
> current tab.

I tested this CL out locally and it appears to have the desired behaviors.
Thanks! lgtm.

[shahriar, 2016-12-20 00:25:52]

On 2016/12/19 17:38:42, erikchen wrote:
> On 2016/12/18 22:31:35, shahriar wrote:
> > On 2016/12/18 19:20:15, Avi wrote:
> > > On 2016/12/18 15:42:06, shahriar wrote:
> > > > Hi Erik
> > > > 
> > > > I am sorry for being so late on this patch.
> > > > 
> > > > I followed the instructions you provide, so the first patch contains all
> the
> > > > previous LGTMed changes, no line added, no line deleted (Bug: 661765),
so
> > you
> > > > might not need to review it again.
> > > > 
> > > > Second patch, contains my fix on bug: 672805.
> > > > 
> > > > PTAL.
> > > 
> > > That bug was dropping onto the omnibox, which I imagine is the same as
> > dropping
> > > on a tab.
> > > 
> > > Would this change force the first URL to open in the current tab even for
a
> > drop
> > > on a vacant area of the tab strip? That doesn't seem right.
> > > 
> > > There are three drop locations I can see here: On the omnibox, on a tab,
on
> a
> > > vacant area of the tab strip. There are drops of single items and multiple
> > > items. So we need to do the right thing in at least six cases, so we need
to
> > > determine what the right thing to do in those cases are, and then test
them.
> > 
> > avi@ to answer your question, this change does not force the first URL to
open
> > in the current tab
> > when dropping on a vacant area of tab strip. 
> > 
> > The only case that the current tab would be affected, is that you drop on
> > toolbar. In that case no matter how
> > many items are in the bookmark folder, the first bookmark would be opened in
> the
> > current tab.
> 
> I tested this CL out locally and it appears to have the desired behaviors.
> Thanks! lgtm.

Thanks erikchen@ for the review. Before I commit, I also need avi@'s review/lgtm
as the owner.

[Avi (use Gerrit), 2016-12-20 02:42:09]

lgtm

Stamp

[shahriar, 2016-12-20 04:51:27]

The CQ bit was checked by shahriar.rostami@gmail.com

[commit-bot: I haz the power, 2016-12-20 04:51:51]

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

[commit-bot: I haz the power, 2016-12-20 05:09:47]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482209487385220,
"parent_rev": "cc4139c3db0db3c8d0e02232a64d697a08d0b753", "commit_rev":
"4661f04939d2b4a0e90043ec1901a55b4b91fb49"}

[commit-bot: I haz the power, 2016-12-20 05:10:59]

Description was changed from

==========
Fixed dragging a folder from bookmark manager to open all elements in new tabs

Dropping a bookmark folder on Toolbar or Tab strip will open all of the urls in
separate tabs.

The first URL of dragged/dropped bookmark on toolbar_controller should be opened
in the current opening tab.

BUG=661765
==========

to

==========
Fixed dragging a folder from bookmark manager to open all elements in new tabs

Dropping a bookmark folder on Toolbar or Tab strip will open all of the urls in
separate tabs.

The first URL of dragged/dropped bookmark on toolbar_controller should be opened
in the current opening tab.

BUG=661765

Review-Url: https://codereview.chromium.org/2587763002
==========

[commit-bot: I haz the power, 2016-12-20 05:11:00]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-20 05:13:57]

Description was changed from

==========
Fixed dragging a folder from bookmark manager to open all elements in new tabs

Dropping a bookmark folder on Toolbar or Tab strip will open all of the urls in
separate tabs.

The first URL of dragged/dropped bookmark on toolbar_controller should be opened
in the current opening tab.

BUG=661765

Review-Url: https://codereview.chromium.org/2587763002
==========

to

==========
Fixed dragging a folder from bookmark manager to open all elements in new tabs

Dropping a bookmark folder on Toolbar or Tab strip will open all of the urls in
separate tabs.

The first URL of dragged/dropped bookmark on toolbar_controller should be opened
in the current opening tab.

BUG=661765

Committed: https://crrev.com/82ba88e6bff66deb3b4a9c06e966637538cd9f89
Cr-Commit-Position: refs/heads/master@{#439713}
==========

[commit-bot: I haz the power, 2016-12-20 05:13:58]

Patchset 2 (id:??) landed as
https://crrev.com/82ba88e6bff66deb3b4a9c06e966637538cd9f89
Cr-Commit-Position: refs/heads/master@{#439713}