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 Omnibox or Tab strip will open all of the urls in
separate tabs.

Note that this does not include urls within nested folders, because
BookmarkNodeData.WriteToClipboard (bookmark_node_data.cc:190) does not put urls
from nested folders into clipboard.

I have two questions from reviewers:

1) Do we have to record metrics in ToolbarController like what we have in
TabStripController.OpenUrl()?

2) I could not find any unit test for testing DropUrls of tab_strip_controller.mm
and toolbar_controller.mm, am I right?

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

[shahriar, 2016-11-13 21:56:08]

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.

Note that this does not include urls within nested folders, because
BookmarkNodeData.WriteToClipboard (bookmark_node_data.cc:190) does not put urls
from nested folders into clipboard.

I have two questions from reviewers:

1) Do we have to record metrics in ToolbarController like what we have in
TabStripController.OpenUrl()?

2) I could not find any unit test for testing DropUrls of
tab_strip_controller.mm
and toolbar_controller.mm, am I right?

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

to

==========
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.

Note that this does not include urls within nested folders, because
BookmarkNodeData.WriteToClipboard (bookmark_node_data.cc:190) does not put urls
from nested folders into clipboard.

I have two questions from reviewers:

1) Do we have to record metrics in ToolbarController like what we have in
TabStripController.OpenUrl()?

2) I could not find any unit test for testing DropUrls of
tab_strip_controller.mm
and toolbar_controller.mm, am I right?

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

[shahriar, 2016-11-13 21:56:08]

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

[shahriar, 2016-11-13 21:57:58]

shahriar.rostami@gmail.com changed reviewers:
+ shrike@chromium.org

[Avi (use Gerrit), 2016-11-13 22:55:55]

avi@chromium.org changed reviewers:
+ erikchen@chromium.org
- rohitrao@chromium.org

[Avi (use Gerrit), 2016-11-13 22:55:56]

Switching out Erik for Rohit, and Rohit hasn't worked in this area for a while.

Erik, you've been in here more recently than I have. WDYT? This looks
reasonable.

https://codereview.chromium.org/2502483002/diff/1/chrome/browser/ui/cocoa/tab...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/2502483002/diff/1/chrome/browser/ui/cocoa/tab...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2112: 
FixupURL returns a GURL. That original code makes no sense; can we do

GURL url = url_formatter::FixupURL(base::SysNSStringToUTF8(urlString),
std::string());

?

https://codereview.chromium.org/2502483002/diff/1/chrome/browser/ui/cocoa/too...
File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right):

https://codereview.chromium.org/2502483002/diff/1/chrome/browser/ui/cocoa/too...
chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:1033: std::string())));
Same as above.

https://codereview.chromium.org/2502483002/diff/1/chrome/browser/ui/cocoa/too...
chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:1038: 
You lost the anti-self-XSS code.

[shahriar, 2016-11-13 23:40:34]

https://codereview.chromium.org/2502483002/diff/1/chrome/browser/ui/cocoa/tab...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/2502483002/diff/1/chrome/browser/ui/cocoa/tab...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2112: 
On 2016/11/13 22:55:55, Avi wrote:
> FixupURL returns a GURL. That original code makes no sense; can we do
> 
> GURL url = url_formatter::FixupURL(base::SysNSStringToUTF8(urlString),
> std::string());
> 
> ?

Done.

https://codereview.chromium.org/2502483002/diff/1/chrome/browser/ui/cocoa/too...
File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right):

https://codereview.chromium.org/2502483002/diff/1/chrome/browser/ui/cocoa/too...
chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:1033: std::string())));
On 2016/11/13 22:55:56, Avi wrote:
> Same as above.

Done.

https://codereview.chromium.org/2502483002/diff/1/chrome/browser/ui/cocoa/too...
chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:1038: 
On 2016/11/13 22:55:55, Avi wrote:
> You lost the anti-self-XSS code.

Agree, I removed it because I thought we are no longer directly updating the
window title using SetUserText, 
so there wouldn't be a need for that check. I fixed it.

[erikchen, 2016-11-14 18:22:53]

Thanks for taking this on. Your code looks great!

https://codereview.chromium.org/2502483002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/2502483002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2120:
tabStripModel_->ActivateTabAt(tabStripModel_->count() - 1, true);
is this necessary? This assumes that the last tab is one of the newly opened
tabs. It also didn't appear necessary before.

https://codereview.chromium.org/2502483002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right):

https://codereview.chromium.org/2502483002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:1031: for (id urlString in
urls) {
This code looks awfully similar to the previous changed code. I won't make you
refactor it, since the existing code was also copy-pasted. However, I would
appreciate it if you could:

1) File a bug indicating that this code needs to be refactored.
2) Add a comment here and in the previous file that indicates that the code
should be refactored:

Something like: // Refactor this code.
https://crbug.com/<insert_new_bug_id_here>.

[shahriar, 2016-11-15 02:51:12]

https://codereview.chromium.org/2502483002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/2502483002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2120:
tabStripModel_->ActivateTabAt(tabStripModel_->count() - 1, true);
On 2016/11/14 18:22:53, erikchen wrote:
> is this necessary? This assumes that the last tab is one of the newly opened
> tabs. It also didn't appear necessary before.

Right, I fixed it to select the last tab inserted. I did this, because I want to
keep it consistent with ToolbarController when we do the same drag/drop
operation. Here we are inserting tabs in reverse order to preserve the order of
bookmarks. Previously we don't need it as we insert only on item.

https://codereview.chromium.org/2502483002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right):

https://codereview.chromium.org/2502483002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:1031: for (id urlString in
urls) {
On 2016/11/14 18:22:53, erikchen wrote:
> This code looks awfully similar to the previous changed code. I won't make you
> refactor it, since the existing code was also copy-pasted. However, I would
> appreciate it if you could:
> 
> 1) File a bug indicating that this code needs to be refactored.
> 2) Add a comment here and in the previous file that indicates that the code
> should be refactored:
> 
> Something like: // Refactor this code.
> https://crbug.com/%3Cinsert_new_bug_id_here%3E.

I looked for a common super/parent class, but they don't have it, otherwise I
would pull up the common code. 
If you have a suggestion, I would take your advice and refactor them in that
change request.

[erikchen, 2016-11-18 18:26:21]

https://codereview.chromium.org/2502483002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/2502483002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2127:
tabStripModel_->ActivateTabAt(
There are better ways of doing this.

You want to activate the last tab inserted. In order to do this, you need to
know meta-information [index] about the tabs that were inserted. This
information needs to propagate from openURL:... to here. The previous
implementation assumed that the last tab inserted would be the last tab in the
tab strip model. This implementation assumes that active_index() hasn't changed
since we started inserting tabs, and that tabs get inserted directly after
active_index(). 

I think you should add another parameter activate:(BOOL) to openURL:... and in
that method, call tabStripModel_->ActivateTabAt(index, true); if that parameter
is true. [I haven't looked into how the disposition might influence this, but
it's likely relevant as well].

[erikchen, 2016-11-18 18:26:39]

On 2016/11/18 18:26:21, erikchen wrote:
>
https://codereview.chromium.org/2502483002/diff/40001/chrome/browser/ui/cocoa...
> File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):
> 
>
https://codereview.chromium.org/2502483002/diff/40001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2127:
> tabStripModel_->ActivateTabAt(
> There are better ways of doing this.
> 
> You want to activate the last tab inserted. In order to do this, you need to
> know meta-information [index] about the tabs that were inserted. This
> information needs to propagate from openURL:... to here. The previous
> implementation assumed that the last tab inserted would be the last tab in the
> tab strip model. This implementation assumes that active_index() hasn't
changed
> since we started inserting tabs, and that tabs get inserted directly after
> active_index(). 
> 
> I think you should add another parameter activate:(BOOL) to openURL:... and in
> that method, call tabStripModel_->ActivateTabAt(index, true); if that
parameter
> is true. [I haven't looked into how the disposition might influence this, but
> it's likely relevant as well].

feel free to ping me if I don't respond within 24 hours [excluding weekends]

[shahriar, 2016-11-21 02:52:38]

https://codereview.chromium.org/2502483002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/2502483002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2127:
tabStripModel_->ActivateTabAt(
On 2016/11/18 18:26:21, erikchen wrote:
> There are better ways of doing this.
> 
> You want to activate the last tab inserted. In order to do this, you need to
> know meta-information [index] about the tabs that were inserted. This
> information needs to propagate from openURL:... to here. The previous
> implementation assumed that the last tab inserted would be the last tab in the
> tab strip model. This implementation assumes that active_index() hasn't
changed
> since we started inserting tabs, and that tabs get inserted directly after
> active_index(). 
> 
> I think you should add another parameter activate:(BOOL) to openURL:... and in
> that method, call tabStripModel_->ActivateTabAt(index, true); if that
parameter
> is true. [I haven't looked into how the disposition might influence this, but
> it's likely relevant as well].

Thank you very much for the explanation. I make changes, as you said, but I
found I can rely on disposition, this way I don't have to manually call
activateTabAt(index, true). However I am not sure if I get the concept of
Foreground and Background tab properly. I am looking forward your kind comments
if I am making a mistake here.

[erikchen, 2016-11-21 18:28:40]

thanks, looks good. One last pass for style/nits.

https://codereview.chromium.org/2502483002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/2502483002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:178:
activateTab:(BOOL)enabled;
for consistency, please use activateTab:(BOOL)activateTab

Note that this applies to several changes you've made in this file.

https://codereview.chromium.org/2502483002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2042: if (enabled)
Chromium style requires that multi-line if statements use curly braces.

e.g. 
if (foo) {
  stuff
} else {
  other stuff
}

Note that this applies to several changes you've made in this file.

https://codereview.chromium.org/2502483002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2132: if ([urls
indexOfObject:urlString] == [urls count] - 1)
This code makes the assumption that the urlString only appears once in urls.
Just use a normal for loop? [e.g. NSUinteger i = [urls count] = 1; i>=0....]

[shahriar, 2016-11-22 03:46:30]

https://codereview.chromium.org/2502483002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/2502483002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:178:
activateTab:(BOOL)enabled;
On 2016/11/21 18:28:40, erikchen wrote:
> for consistency, please use activateTab:(BOOL)activateTab
> 
> Note that this applies to several changes you've made in this file.

Done.

https://codereview.chromium.org/2502483002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2042: if (enabled)
On 2016/11/21 18:28:40, erikchen wrote:
> Chromium style requires that multi-line if statements use curly braces.
> 
> e.g. 
> if (foo) {
>   stuff
> } else {
>   other stuff
> }
> 
> Note that this applies to several changes you've made in this file.

I fixed it as you said. But I thought when neither "if" nor "else" is multi
statements, it is allowed to omit them for both.

https://codereview.chromium.org/2502483002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2132: if ([urls
indexOfObject:urlString] == [urls count] - 1)
On 2016/11/21 18:28:40, erikchen wrote:
> This code makes the assumption that the urlString only appears once in urls.
> Just use a normal for loop? [e.g. NSUinteger i = [urls count] = 1; i>=0....]

Done.

[erikchen, 2016-11-22 18:12:48]

looks fine. One last nit. Otherwise lgtm.

https://codereview.chromium.org/2502483002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/2502483002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2042: if (enabled)
On 2016/11/22 03:46:30, shahriar wrote:
> On 2016/11/21 18:28:40, erikchen wrote:
> > Chromium style requires that multi-line if statements use curly braces.
> > 
> > e.g. 
> > if (foo) {
> >   stuff
> > } else {
> >   other stuff
> > }
> > 
> > Note that this applies to several changes you've made in this file.
> 
> I fixed it as you said. But I thought when neither "if" nor "else" is multi
> statements, it is allowed to omit them for both.

Hm. I reread the Google style guide and you're right - this may be one of the
exceptions that doesn't require curly braces:
https://google.github.io/styleguide/cppguide.html#Conditionals

[that being said I still prefer them, I just wouldn't have nitted if I had
known.]

https://codereview.chromium.org/2502483002/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/2502483002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2134: if (index ==
(NSInteger)[urls count] - 1) {
We don't allow C-style casts. You can probably just change index to be an
NSUInteger. Otherwise, use static_cast<NSInteger>

[shahriar, 2016-11-22 22:44:05]

Thanks erikchen@, I learned many things with your reviews.

https://codereview.chromium.org/2502483002/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/2502483002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2134: if (index ==
(NSInteger)[urls count] - 1) {
On 2016/11/22 18:12:48, erikchen wrote:
> We don't allow C-style casts. You can probably just change index to be an
> NSUInteger. Otherwise, use static_cast<NSInteger>

Actually I don't use NSUInteger because it's a reverse for loop, and it requires
to compare index to "0", which is forbidden. As you said, I use
static_cast<NSInteger>(...).

[erikchen, 2016-11-22 22:52:26]

https://codereview.chromium.org/2502483002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/2502483002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2124: for (NSInteger index
= [urls count] - 1; index >= 0; index--) {
Comparison against 0 is fine, you just need to use an unsigned 0 e.g. 
"""
index >= 0u
"""

[erikchen, 2016-11-22 22:52:43]

On 2016/11/22 22:52:26, erikchen wrote:
>
https://codereview.chromium.org/2502483002/diff/100001/chrome/browser/ui/coco...
> File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):
> 
>
https://codereview.chromium.org/2502483002/diff/100001/chrome/browser/ui/coco...
> chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2124: for (NSInteger
index
> = [urls count] - 1; index >= 0; index--) {
> Comparison against 0 is fine, you just need to use an unsigned 0 e.g. 
> """
> index >= 0u
> """

Oh wait. I'm an idiot. It doesn't make sense to compare unsigned integer against
0.

[erikchen, 2016-11-22 22:53:12]

lgtm

[shahriar, 2016-11-23 00:29:10]

The CQ bit was checked by shahriar.rostami@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 00:29:54]

Dry run: 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-11-23 00:57:23]

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

[commit-bot: I haz the power, 2016-11-23 00:57:24]

Dry run: This issue passed the CQ dry run.

[shahriar, 2016-11-23 00:57:46]

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

[commit-bot: I haz the power, 2016-11-23 00:58:42]

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-11-23 01:15:05]

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

[commit-bot: I haz the power, 2016-11-23 01:15:08]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[shahriar, 2016-11-23 03:54:32]

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

[commit-bot: I haz the power, 2016-11-23 03:54:43]

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-11-23 04:02:25]

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

[commit-bot: I haz the power, 2016-11-23 04:02:27]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[shahriar, 2016-11-23 04:07:00]

On 2016/11/23 03:54:43, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

Based on the log files, I found I can't commit, as it requires owners LGTM.

[Avi (use Gerrit), 2016-11-23 04:25:18]

lgtm

If Erik is happy I will rubber stamp.

[Avi (use Gerrit), 2016-11-23 04:25:27]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2016-11-23 04:25:37]

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-11-23 04:29:01]

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

[commit-bot: I haz the power, 2016-11-23 04:29:04]

Failed to apply patch for chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:
While running git apply --index -p1;
  error: patch failed: chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2037
  error: chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm: patch does not
apply

Patch:       chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm
Index: chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm
diff --git a/chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm
b/chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm
index
97eb5f57a8c69e4dc60dc633054ae5fa214548fe..0e15a273c08218d010f0fbbecae4500b49a6efb8
100644
--- a/chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm
+++ b/chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm
@@ -174,7 +174,8 @@ CGFloat FlipXInView(NSView* view, CGFloat width, CGFloat x)
{
 - (void)setTabTrackingAreasEnabled:(BOOL)enabled;
 - (void)droppingURLsAt:(NSPoint)point
             givesIndex:(NSInteger*)index
-           disposition:(WindowOpenDisposition*)disposition;
+           disposition:(WindowOpenDisposition*)disposition
+           activateTab:(BOOL)activateTab;
 - (void)setNewTabButtonHoverState:(BOOL)showHover;
 - (void)themeDidChangeNotification:(NSNotification*)notification;
 - (BOOL)doesAnyOtherWebContents:(content::WebContents*)selected
@@ -2010,7 +2011,8 @@ CGFloat FlipXInView(NSView* view, CGFloat width, CGFloat
x) {
 // to the left, it inserts to the left, and similarly for the right.
 - (void)droppingURLsAt:(NSPoint)point
             givesIndex:(NSInteger*)index
-           disposition:(WindowOpenDisposition*)disposition {
+           disposition:(WindowOpenDisposition*)disposition
+           activateTab:(BOOL)activateTab {
   // Proportion of the tab which is considered the "middle" (and causes things
   // to drop on that tab).
   const double kMiddleProportion = 0.5;
@@ -2037,7 +2039,11 @@ CGFloat FlipXInView(NSView* view, CGFloat width, CGFloat
x) {
     // Drop in a new tab to the left of tab |i|?
     if (point.x < (frame.origin.x + kLRProportion * frame.size.width)) {
       *index = i;
-      *disposition = WindowOpenDisposition::NEW_FOREGROUND_TAB;
+      if (activateTab) {
+        *disposition = WindowOpenDisposition::NEW_FOREGROUND_TAB;
+      } else {
+        *disposition = WindowOpenDisposition::NEW_BACKGROUND_TAB;
+      }
       return;
     }
 
@@ -2056,10 +2062,17 @@ CGFloat FlipXInView(NSView* view, CGFloat width, CGFloat
x) {
 
   // If we've made it here, we want to append a new tab to the end.
   *index = -1;
-  *disposition = WindowOpenDisposition::NEW_FOREGROUND_TAB;
+  if (activateTab) {
+    *disposition = WindowOpenDisposition::NEW_FOREGROUND_TAB;
+  } else {
+    *disposition = WindowOpenDisposition::NEW_BACKGROUND_TAB;
+  }
 }
 
-- (void)openURL:(GURL*)url inView:(NSView*)view at:(NSPoint)point {
+- (void)openURL:(GURL*)url
+         inView:(NSView*)view
+             at:(NSPoint)point
+    activateTab:(BOOL)activateTab {
   // Security: Block JavaScript to prevent self-XSS.
   if (url->SchemeIs(url::kJavaScriptScheme))
     return;
@@ -2069,11 +2082,13 @@ CGFloat FlipXInView(NSView* view, CGFloat width, CGFloat
x) {
   WindowOpenDisposition disposition;
   [self droppingURLsAt:point
             givesIndex:&index
-           disposition:&disposition];
+           disposition:&disposition
+           activateTab:activateTab];
 
   // Either insert a new tab or open in a current tab.
   switch (disposition) {
-    case WindowOpenDisposition::NEW_FOREGROUND_TAB: {
+    case WindowOpenDisposition::NEW_FOREGROUND_TAB:
+    case WindowOpenDisposition::NEW_BACKGROUND_TAB: {
       content::RecordAction(UserMetricsAction("Tab_DropURLBetweenTabs"));
       chrome::NavigateParams params(browser_, *url,
                                     ui::PAGE_TRANSITION_TYPED);
@@ -2106,19 +2121,22 @@ CGFloat FlipXInView(NSView* view, CGFloat width, CGFloat
x) {
     return;
   }
 
-  //TODO(viettrungluu): dropping multiple URLs.
-  if ([urls count] > 1)
-    NOTIMPLEMENTED();
-
-  // Get the first URL and fix it up.
-  GURL url(GURL(url_formatter::FixupURL(
-      base::SysNSStringToUTF8([urls objectAtIndex:0]), std::string())));
+  for (NSInteger index = [urls count] - 1; index >= 0; index--) {
+    // Refactor this code.
+    // https://crbug.com/665261.
+    GURL url = url_formatter::FixupURL(
+        base::SysNSStringToUTF8([urls objectAtIndex:index]), std::string());
 
-  // If the URL isn't valid, don't bother.
-  if (!url.is_valid())
-    return;
+    // If the URL isn't valid, don't bother.
+    if (!url.is_valid())
+      continue;
 
-  [self openURL:&url inView:view at:point];
+    if (index == static_cast<NSInteger>([urls count]) - 1) {
+      [self openURL:&url inView:view at:point activateTab:YES];
+    } else {
+      [self openURL:&url inView:view at:point activateTab:NO];
+    }
+  }
 }
 
 // (URLDropTargetController protocol)
@@ -2132,7 +2150,7 @@ CGFloat FlipXInView(NSView* view, CGFloat width, CGFloat
x) {
       metrics::OmniboxEventProto::BLANK, &match, NULL);
   GURL url(match.destination_url);
 
-  [self openURL:&url inView:view at:point];
+  [self openURL:&url inView:view at:point activateTab:YES];
 }
 
 // (URLDropTargetController protocol)
@@ -2147,7 +2165,8 @@ CGFloat FlipXInView(NSView* view, CGFloat width, CGFloat
x) {
   WindowOpenDisposition disposition;
   [self droppingURLsAt:point
             givesIndex:&index
-           disposition:&disposition];
+           disposition:&disposition
+           activateTab:YES];
 
   NSPoint arrowPos = NSMakePoint(0, arrowBaseY);
   if (index == -1) {

[shahriar, 2016-11-23 04:36:59]

On 2016/11/23 04:25:37, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

Avi@ thanks, is the failure because of conflict? I don't get what exactly
happened.

[Avi (use Gerrit), 2016-11-23 04:52:47]

On 2016/11/23 04:36:59, shahriar wrote:
> On 2016/11/23 04:25:37, commit-bot: I haz the power wrote:
> > CQ is trying da patch. Follow status at
> >  
> >
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> 
> Avi@ thanks, is the failure because of conflict? I don't get what exactly
> happened.

Yes, the tree changed since you last updated. Do a git rebase-update, resolve
the conflicts, re-upload, and try committing again.

[shahriar, 2016-11-23 05:11:20]

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

[shahriar, 2016-11-23 05:11:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
erikchen@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2502483002/#ps120001
(title: "Replace c-style cast with static_cast for drag/drop bookmarks over tab
strip")

[commit-bot: I haz the power, 2016-11-23 05:11:39]

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-11-23 05:41:34]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1479877880904540,
"parent_rev": "20aeb976aa6371fc16722d2dda90f8e827bf57a3", "commit_rev":
"94fab7f4e1d214c3fa0e82f34e9088f26003e4be"}

[commit-bot: I haz the power, 2016-11-23 05:41:57]

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.

Note that this does not include urls within nested folders, because
BookmarkNodeData.WriteToClipboard (bookmark_node_data.cc:190) does not put urls
from nested folders into clipboard.

I have two questions from reviewers:

1) Do we have to record metrics in ToolbarController like what we have in
TabStripController.OpenUrl()?

2) I could not find any unit test for testing DropUrls of
tab_strip_controller.mm
and toolbar_controller.mm, am I right?

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

to

==========
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.

Note that this does not include urls within nested folders, because
BookmarkNodeData.WriteToClipboard (bookmark_node_data.cc:190) does not put urls
from nested folders into clipboard.

I have two questions from reviewers:

1) Do we have to record metrics in ToolbarController like what we have in
TabStripController.OpenUrl()?

2) I could not find any unit test for testing DropUrls of
tab_strip_controller.mm
and toolbar_controller.mm, am I right?

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

[commit-bot: I haz the power, 2016-11-23 05:42:00]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-11-23 05:44:18]

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.

Note that this does not include urls within nested folders, because
BookmarkNodeData.WriteToClipboard (bookmark_node_data.cc:190) does not put urls
from nested folders into clipboard.

I have two questions from reviewers:

1) Do we have to record metrics in ToolbarController like what we have in
TabStripController.OpenUrl()?

2) I could not find any unit test for testing DropUrls of
tab_strip_controller.mm
and toolbar_controller.mm, am I right?

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

to

==========
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.

Note that this does not include urls within nested folders, because
BookmarkNodeData.WriteToClipboard (bookmark_node_data.cc:190) does not put urls
from nested folders into clipboard.

I have two questions from reviewers:

1) Do we have to record metrics in ToolbarController like what we have in
TabStripController.OpenUrl()?

2) I could not find any unit test for testing DropUrls of
tab_strip_controller.mm
and toolbar_controller.mm, am I right?

BUG=661765

Committed: https://crrev.com/d2e643efc4cceed77e046658a588f1559f1b1e88
Cr-Commit-Position: refs/heads/master@{#434124}
==========

[commit-bot: I haz the power, 2016-11-23 05:44:19]

Patchset 7 (id:??) landed as
https://crrev.com/d2e643efc4cceed77e046658a588f1559f1b1e88
Cr-Commit-Position: refs/heads/master@{#434124}

[Mark P, 2016-11-23 19:19:47]

On 2016/11/23 05:44:19, commit-bot: I haz the power wrote:
> Patchset 7 (id:??) landed as
> https://crrev.com/d2e643efc4cceed77e046658a588f1559f1b1e88
> Cr-Commit-Position: refs/heads/master@{#434124}

I don't see any discussion (maybe I missed it) of this question in the
changelist description:
"Do we have to record metrics in ToolbarController like what we have in
TabStripController.OpenUrl()?"

As someone who works often with metrics, I'm curious about why you decided it
was not appropriate to record metrics in ToolbarController?

thanks,
mark

[shahriar, 2016-11-24 00:53:42]

On 2016/11/23 19:19:47, Mark P (out Nov 17-20) wrote:
> On 2016/11/23 05:44:19, commit-bot: I haz the power wrote:
> > Patchset 7 (id:??) landed as
> > https://crrev.com/d2e643efc4cceed77e046658a588f1559f1b1e88
> > Cr-Commit-Position: refs/heads/master@{#434124}
> 
> I don't see any discussion (maybe I missed it) of this question in the
> changelist description:
> "Do we have to record metrics in ToolbarController like what we have in
> TabStripController.OpenUrl()?"
> 
> As someone who works often with metrics, I'm curious about why you decided it
> was not appropriate to record metrics in ToolbarController?
> 
> thanks,
> mark

Mark, actually I noticed we are not recording metrics in ToolbarController for
dropping urls
(even prior to this change), and I asked that question because I was not sure if
it's intentional or not.
So, as a workaround, if you think we should record the metrics, I can create an
issue to investigate it.

[Mark P, 2016-11-28 22:42:47]

On 2016/11/24 00:53:42, shahriar wrote:
> On 2016/11/23 19:19:47, Mark P (out Nov 17-20) wrote:
> > On 2016/11/23 05:44:19, commit-bot: I haz the power wrote:
> > > Patchset 7 (id:??) landed as
> > > https://crrev.com/d2e643efc4cceed77e046658a588f1559f1b1e88
> > > Cr-Commit-Position: refs/heads/master@{#434124}
> > 
> > I don't see any discussion (maybe I missed it) of this question in the
> > changelist description:
> > "Do we have to record metrics in ToolbarController like what we have in
> > TabStripController.OpenUrl()?"
> > 
> > As someone who works often with metrics, I'm curious about why you decided
it
> > was not appropriate to record metrics in ToolbarController?
> > 
> > thanks,
> > mark
> 
> Mark, actually I noticed we are not recording metrics in ToolbarController for
> dropping urls
> (even prior to this change), and I asked that question because I was not sure
if
> it's intentional or not.
> So, as a workaround, if you think we should record the metrics, I can create
an
> issue to investigate it.

I tend to think all meaningful user actions should be logged somehow.  Please
create
an issue.  I don't know how important adding these metrics are (probably not
very, i.e.,
P3).  And I don't know the current situation well: if we're logging a user
action along
one code path but not a very similar (or identcal) user action which happens to
hit a
different code path, that's worse than missing logging entirely.  (The
inconsistency
can easily lead to wrong conclusions by people who don't know better.)

--mark

[Mark P, 2016-11-28 22:42:59]

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.

Note that this does not include urls within nested folders, because
BookmarkNodeData.WriteToClipboard (bookmark_node_data.cc:190) does not put urls
from nested folders into clipboard.

I have two questions from reviewers:

1) Do we have to record metrics in ToolbarController like what we have in
TabStripController.OpenUrl()?

2) I could not find any unit test for testing DropUrls of
tab_strip_controller.mm
and toolbar_controller.mm, am I right?

BUG=661765

Committed: https://crrev.com/d2e643efc4cceed77e046658a588f1559f1b1e88
Cr-Commit-Position: refs/heads/master@{#434124}
==========

to

==========
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.

Note that this does not include urls within nested folders, because
BookmarkNodeData.WriteToClipboard (bookmark_node_data.cc:190) does not put urls
from nested folders into clipboard.

I have two questions from reviewers:

1) Do we have to record metrics in ToolbarController like what we have in
TabStripController.OpenUrl()?

2) I could not find any unit test for testing DropUrls of
tab_strip_controller.mm
and toolbar_controller.mm, am I right?

BUG=661765

Committed: https://crrev.com/d2e643efc4cceed77e046658a588f1559f1b1e88
Cr-Commit-Position: refs/heads/master@{#434124}
==========

[erikchen, 2016-12-15 01:46:34]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2572363002/ by erikchen@chromium.org.

The reason for reverting is: Reverting because this introduces a bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=672805.