Prevent chrome.windows.create from moving a tab to another profile

chrome/browser/extensions/api/tabs/tabs_api.cc

Index: chrome/browser/extensions/api/tabs/tabs_api.cc
diff --git a/chrome/browser/extensions/api/tabs/tabs_api.cc b/chrome/browser/extensions/api/tabs/tabs_api.cc
index f2d0fd9daab8032058b3663bc8e7f703fe6e2c0e..47905f733a4f15e4a08ceb47efc1b81a5eae0248 100644
--- a/chrome/browser/extensions/api/tabs/tabs_api.cc
+++ b/chrome/browser/extensions/api/tabs/tabs_api.cc
@@ -459,19 +459,40 @@ bool WindowsCreateFunction::RunSync() {
     }
   }
 
+  // Decide whether we are opening a normal window or an incognito window.
+  bool is_error = true;
+  bool open_incognito_window = ShouldOpenIncognitoWindow(create_data, &urls,
+                                                         &is_error);
+  if (is_error)
+    return false;  // error_ member is set inside of ShouldOpenIncognitoWindow.

[Finnur, 2016/05/17 14:49:14]

nit: This comment doesn't really add anything, I wouldn't retain it.

[Devlin, 2016/05/17 19:29:54]

I disagree a little - it explains why we don't have to set the error_ message
separately here (not to be mistaken with is_error).  If I'm just looking at this
code, I should be on the look out for returning false without having set error_
appropriately.

[Finnur, 2016/05/18 14:06:32]

Oh, error_member! Sorry, I read this as:

// is_error is set inside of ShouldOpenIncognitoWindow.

My bad. Fine to keep it.

+
   // Look for optional tab id.
   if (create_data && create_data->tab_id) {
     // Find the tab. |source_tab_strip| and |tab_index| will later be used to
     // move the tab into the created window.
+    Browser* source_browser = nullptr;

[Devlin, 2016/05/17 19:29:54]

nit: source_browser, to me, implies that this is the browser that sent the
message.  Maybe "tabs_old_browser"?  "move_tab_from_browser"?  Something along
those lines?

[robwu, 2016/05/17 21:14:24]

source as in source of the tab. I'm using source_* to be consistent with
"source_tab_strip". And also with MoveTab.

[Devlin, 2016/05/18 17:09:41]

I still disagree with the naming, but fair point regarding consistency.  This is
fine.

     if (!GetTabById(*create_data->tab_id,
                     GetProfile(),
                     include_incognito(),
-                    NULL,
+                    &source_browser,
                     &source_tab_strip,
-                    NULL,
+                    nullptr,
                     &tab_index,
                     &error_))
       return false;
+
+    if (!source_browser->window()->IsTabStripEditable()) {
+      error_ = keys::kTabStripNotEditableError;
+      return false;
+    }
+
+    bool is_source_incognito =
+        source_browser->profile()->GetProfileType() ==
+        Profile::INCOGNITO_PROFILE;

[Devlin, 2016/05/17 19:29:54]

nit: I think we typically use BrowserContext::IsOffTheRecord() for this check
(so here, just profile()->IsOffTheRecord()).  Is there a reason to prefer
GetProfileType()?

+    if (open_incognito_window != is_source_incognito) {
+      error_ = keys::kCanOnlyMoveTabsWithinSameProfileError;

[Devlin, 2016/05/17 19:29:54]

I also wonder if there's some other tricky edge case we might potentially want
to avoid in the future (I can't think of any yet, but the types of profiles we
have are growing).  Maybe we could just do:
if (window_profile != tabs_profile) {
  // Abort
}
?

It seems like that's really what we want to avoid, so it'd make sense to make
that the direct check.

(That would mean moving window_profile around, but I think that's fine)

[robwu, 2016/05/17 21:14:24]

Done.

+      return false;
+    }
   }
 
   if (!IsValidStateForWindowsCreateFunction(create_data)) {
@@ -490,15 +511,6 @@ bool WindowsCreateFunction::RunSync() {
   bool focused = true;
   bool saw_focus_key = false;
   std::string extension_id;
-
-  // Decide whether we are opening a normal window or an incognito window.
-  bool is_error = true;
-  bool open_incognito_window = ShouldOpenIncognitoWindow(create_data, &urls,
-                                                         &is_error);
-  if (is_error) {
-    // error_ member variable is set inside of ShouldOpenIncognitoWindow.
-    return false;
-  }
   if (open_incognito_window) {
     window_profile = window_profile->GetOffTheRecordProfile();
   }