Allow extensions to pin tabs:...

Allow extensions to pin tabs:

- Add 'pinned (boolean)' property to the Tab object.
- Allow the updateProperties parameter of chrome.tabs.update() to have the 'pinned' property.
- Allow the createProperties parameter of chrome.tabs.create() to have the property as well.

BUG=24781
TEST=run the browser_test.

[Erik does not do reviews, 2009-10-14 16:25:29]

Please hold onto this patch for now.  The "pinned" behavior in Chrome is still
in the experimental stage.  Until the UI and behavior are final, we shouldn't
land extension API to control it.

http://codereview.chromium.org/273048/diff/1/2
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/273048/diff/1/2#newcode527
Line 527: bool pinned = false;
move this inside the if

http://codereview.chromium.org/273048/diff/1/2#newcode531
Line 531: // If the "pinned" key is not found, keep the current state.
remove this comment (it's saying the opposite of what it does, and what it does
is pretty easy to see)

http://codereview.chromium.org/273048/diff/1/2#newcode631
Line 631: bool pinned = false;
move inside the if

http://codereview.chromium.org/273048/diff/1/2#newcode636
Line 636: // If the "pinned" key is not found, keep the current state.
remove comment

http://codereview.chromium.org/273048/diff/1/5
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/273048/diff/1/5#newcode468
Line 468: "description": "Whether the tab should become pinned. Defaults to
<var>false</var>"
remove "defaults to false".  It really defaults to "undefined" which means keep
the current state.

[Yusuke Sato, 2009-10-15 09:20:21]

Erik, thanks for the review. Addressed all issues.

On 2009/10/14 16:25:29, Erik Kay wrote:
> Please hold onto this patch for now.  The "pinned" behavior in Chrome is still
> in the experimental stage.  Until the UI and behavior are final, we shouldn't
> land extension API to control it.
> 
> http://codereview.chromium.org/273048/diff/1/2
> File chrome/browser/extensions/extension_tabs_module.cc (right):
> 
> http://codereview.chromium.org/273048/diff/1/2#newcode527
> Line 527: bool pinned = false;
> move this inside the if
> 
> http://codereview.chromium.org/273048/diff/1/2#newcode531
> Line 531: // If the "pinned" key is not found, keep the current state.
> remove this comment (it's saying the opposite of what it does, and what it
does
> is pretty easy to see)
> 
> http://codereview.chromium.org/273048/diff/1/2#newcode631
> Line 631: bool pinned = false;
> move inside the if
> 
> http://codereview.chromium.org/273048/diff/1/2#newcode636
> Line 636: // If the "pinned" key is not found, keep the current state.
> remove comment
> 
> http://codereview.chromium.org/273048/diff/1/5
> File chrome/common/extensions/api/extension_api.json (right):
> 
> http://codereview.chromium.org/273048/diff/1/5#newcode468
> Line 468: "description": "Whether the tab should become pinned. Defaults to
> <var>false</var>"
> remove "defaults to false".  It really defaults to "undefined" which means
keep
> the current state.

[Yusuke Sato, 2009-10-15 10:47:57]

sorry, I forgot to publish my comments :(

http://codereview.chromium.org/273048/diff/1/2
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/273048/diff/1/2#newcode527
Line 527: bool pinned = false;
On 2009/10/14 16:25:30, Erik Kay wrote:
> move this inside the if

Done.

http://codereview.chromium.org/273048/diff/1/2#newcode531
Line 531: // If the "pinned" key is not found, keep the current state.
On 2009/10/14 16:25:30, Erik Kay wrote:
> remove this comment (it's saying the opposite of what it does, and what it
does
> is pretty easy to see)

Done.

http://codereview.chromium.org/273048/diff/1/2#newcode631
Line 631: bool pinned = false;
On 2009/10/14 16:25:30, Erik Kay wrote:
> move inside the if

Done.

http://codereview.chromium.org/273048/diff/1/2#newcode636
Line 636: // If the "pinned" key is not found, keep the current state.
On 2009/10/14 16:25:30, Erik Kay wrote:
> remove comment

Done.

http://codereview.chromium.org/273048/diff/1/5
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/273048/diff/1/5#newcode468
Line 468: "description": "Whether the tab should become pinned. Defaults to
<var>false</var>"
On 2009/10/14 16:25:30, Erik Kay wrote:
> remove "defaults to false".  It really defaults to "undefined" which means
keep
> the current state.

Done.

[Erik does not do reviews, 2009-10-15 18:40:56]

cool.  Looks good.  Like I said, please don't commit this until we decide that
the pinning UI is finalized for 4.0.

[Yusuke Sato, 2009-11-02 14:10:53]

Can we commit this change?

The code freeze day for 4.0 is closing, and (as far as I know) no UI change that
could affect this change is proposed.

[lucabelluccini, 2009-12-08 10:48:23]

What's the status for this feature?

On 2009/11/02 14:10:53, Yusuke Sato wrote:
> Can we commit this change?
> 
> The code freeze day for 4.0 is closing, and (as far as I know) no UI change
that
> could affect this change is proposed.

[lucabelluccini, 2009-12-08 14:31:43]

I am developing an extension
(http://www.screentoaster.com/watch/stUk1dSkVLQ1ldSF9fW1Jf) which allows to set
some of the current tabs as pinned each time a chrome window is opened.

On 2009/12/08 10:48:23, lucabelluccini wrote:
> What's the status for this feature?
> 
> On 2009/11/02 14:10:53, Yusuke Sato wrote:
> > Can we commit this change?
> > 
> > The code freeze day for 4.0 is closing, and (as far as I know) no UI change
> that
> > could affect this change is proposed.

[Aaron Boodman, 2010-10-14 18:04:17]

http://codereview.chromium.org/273048/diff/4001/5001
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/273048/diff/4001/5001#newcode531
chrome/browser/extensions/extension_tabs_module.cc:531:
tab_strip->SetTabPinned(index, pinned);
This part of the code has changed since you wrote the change, and you'll have to
update it slightly.

There is now a sort of TabConfig structure that you pass into AddTab() that
contains all the information about the tab to be created. Set pinned on that
before creating tab.

http://codereview.chromium.org/273048/diff/4001/5001#newcode635
chrome/browser/extensions/extension_tabs_module.cc:635:
tab_strip->SetTabPinned(tab_index, pinned);
Same here.

http://codereview.chromium.org/273048/diff/4001/5005
File chrome/renderer/extensions/extension_api_client_unittest.cc (right):

http://codereview.chromium.org/273048/diff/4001/5005#newcode386
chrome/renderer/extensions/extension_api_client_unittest.cc:386:
ExpectJsFail("chrome.tabs.update(42, {pinned: 'foo'});",
We don't really use this class anymore. We should actually remove it... But we
should add to extension_tabs_apitest.cc to exercise the api.