Mac Web Intents Part 11: Progress view

Mac Web Intents Part 11: Progress view

This CL adds a progress view to the web intents picker dialog. The view is used to display the "waiting for chrome web store" and "installing extension" progress.

Screenshot: http://i.imgur.com/riMRo.png

BUG=152010
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161302

[Nico, 2012-10-02 03:47:25]

lgtm once rebased on top of the rest

http://codereview.chromium.org/11009017/diff/1015/chrome/browser/ui/cocoa/int...
File chrome/browser/ui/cocoa/intents/web_intent_picker_view_controller.h
(right):

http://codereview.chromium.org/11009017/diff/1015/chrome/browser/ui/cocoa/int...
chrome/browser/ui/cocoa/intents/web_intent_picker_view_controller.h:20:
PICKER_STATE_INSTALLING_EXTENSION,
(this value might not belong to this cl? not an issue tho)

http://codereview.chromium.org/11009017/diff/1015/chrome/browser/ui/cocoa/int...
chrome/browser/ui/cocoa/intents/web_intent_picker_view_controller.h:20:
PICKER_STATE_INSTALLING_EXTENSION,
Would these enums be useful for the other UIs too? Should the controller know
about them and send "show the view corresponding to this enum" messages to the
UI code instead?

http://codereview.chromium.org/11009017/diff/1015/chrome/browser/ui/cocoa/int...
File chrome/browser/ui/cocoa/intents/web_intent_view_controller_progress.h
(right):

http://codereview.chromium.org/11009017/diff/1015/chrome/browser/ui/cocoa/int...
chrome/browser/ui/cocoa/intents/web_intent_view_controller_progress.h:17:
@interface WebIntentViewControllerProgress : NSViewController
As I mentioned in an earlier CL of this saga, I think
WebIntentProgressViewController is a less ambiguous name

http://codereview.chromium.org/11009017/diff/1015/chrome/browser/ui/cocoa/int...
File chrome/browser/ui/cocoa/intents/web_intent_view_controller_progress.mm
(right):

http://codereview.chromium.org/11009017/diff/1015/chrome/browser/ui/cocoa/int...
chrome/browser/ui/cocoa/intents/web_intent_view_controller_progress.mm:35:
[result appendAttributedString:string2];
ugh!

(I don't know a shorter way to do this either though.)

I suppose you could change this to take an array of nsattribstrings instead and
put it in some common place. No need to do that at the moment though.

http://codereview.chromium.org/11009017/diff/1015/chrome/browser/ui/cocoa/spi...
File chrome/browser/ui/cocoa/spinner_progress_indicator.h (right):

http://codereview.chromium.org/11009017/diff/1015/chrome/browser/ui/cocoa/spi...
chrome/browser/ui/cocoa/spinner_progress_indicator.h:10: #include
"base/memory/scoped_nsobject.h"
not needed

[groby-ooo-7-16, 2012-10-02 18:35:15]

lgtm

[Robert Sesek, 2012-10-03 19:11:44]

http://codereview.chromium.org/11009017/diff/1015/chrome/browser/ui/cocoa/int...
File chrome/browser/ui/cocoa/intents/web_intent_view_controller_progress.mm
(right):

http://codereview.chromium.org/11009017/diff/1015/chrome/browser/ui/cocoa/int...
chrome/browser/ui/cocoa/intents/web_intent_view_controller_progress.mm:25:
return scoped_nsobject<NSAttributedString>([string2 retain]);
This returns a strong reference while the |result| is autoreleased?

[sail, 2012-10-10 02:00:10]

addressed all review comments, please take another look

http://codereview.chromium.org/11009017/diff/1015/chrome/browser/ui/cocoa/int...
File chrome/browser/ui/cocoa/intents/web_intent_picker_view_controller.h
(right):

http://codereview.chromium.org/11009017/diff/1015/chrome/browser/ui/cocoa/int...
chrome/browser/ui/cocoa/intents/web_intent_picker_view_controller.h:20:
PICKER_STATE_INSTALLING_EXTENSION,
On 2012/10/02 03:47:25, Nico wrote:
> (this value might not belong to this cl? not an issue tho)

The progress UI is used for two states, waiting and installing extension.

http://codereview.chromium.org/11009017/diff/1015/chrome/browser/ui/cocoa/int...
chrome/browser/ui/cocoa/intents/web_intent_picker_view_controller.h:20:
PICKER_STATE_INSTALLING_EXTENSION,
On 2012/10/02 03:47:25, Nico wrote:
> Would these enums be useful for the other UIs too? Should the controller know
> about them and send "show the view corresponding to this enum" messages to the
> UI code instead?

Not sure, I think Rachel and Greg are still doing other refactoring.

http://codereview.chromium.org/11009017/diff/1015/chrome/browser/ui/cocoa/int...
File chrome/browser/ui/cocoa/intents/web_intent_view_controller_progress.h
(right):

http://codereview.chromium.org/11009017/diff/1015/chrome/browser/ui/cocoa/int...
chrome/browser/ui/cocoa/intents/web_intent_view_controller_progress.h:17:
@interface WebIntentViewControllerProgress : NSViewController
On 2012/10/02 03:47:25, Nico wrote:
> As I mentioned in an earlier CL of this saga, I think
> WebIntentProgressViewController is a less ambiguous name

Done.

http://codereview.chromium.org/11009017/diff/1015/chrome/browser/ui/cocoa/int...
File chrome/browser/ui/cocoa/intents/web_intent_view_controller_progress.mm
(right):

http://codereview.chromium.org/11009017/diff/1015/chrome/browser/ui/cocoa/int...
chrome/browser/ui/cocoa/intents/web_intent_view_controller_progress.mm:25:
return scoped_nsobject<NSAttributedString>([string2 retain]);
On 2012/10/03 19:11:44, rsesek wrote:
> This returns a strong reference while the |result| is autoreleased?

Done.
Oops, fixed.

http://codereview.chromium.org/11009017/diff/1015/chrome/browser/ui/cocoa/spi...
File chrome/browser/ui/cocoa/spinner_progress_indicator.h (right):

http://codereview.chromium.org/11009017/diff/1015/chrome/browser/ui/cocoa/spi...
chrome/browser/ui/cocoa/spinner_progress_indicator.h:10: #include
"base/memory/scoped_nsobject.h"
On 2012/10/02 03:47:25, Nico wrote:
> not needed

Done.

[asanka, 2012-10-10 21:15:21]

chrome/browser/download LGTM

[Robert Sesek, 2012-10-10 21:29:35]

http://codereview.chromium.org/11009017/diff/21021/chrome/browser/ui/cocoa/in...
File
chrome/browser/ui/cocoa/intents/web_intent_progress_view_controller_unittest.mm
(right):

http://codereview.chromium.org/11009017/diff/21021/chrome/browser/ui/cocoa/in...
chrome/browser/ui/cocoa/intents/web_intent_progress_view_controller_unittest.mm:23:
MessageLoop message_loop_;
What's this for?

http://codereview.chromium.org/11009017/diff/21021/content/shell/shell_browse...
File content/shell/shell_browser_main_parts_mac.mm (left):

http://codereview.chromium.org/11009017/diff/21021/content/shell/shell_browse...
content/shell/shell_browser_main_parts_mac.mm:5: #include
"content/shell/shell_browser_main_parts.h"
?

[sail, 2012-10-10 21:39:09]

http://codereview.chromium.org/11009017/diff/21021/chrome/browser/ui/cocoa/in...
File
chrome/browser/ui/cocoa/intents/web_intent_progress_view_controller_unittest.mm
(right):

http://codereview.chromium.org/11009017/diff/21021/chrome/browser/ui/cocoa/in...
chrome/browser/ui/cocoa/intents/web_intent_progress_view_controller_unittest.mm:23:
MessageLoop message_loop_;
On 2012/10/10 21:29:36, rsesek wrote:
> What's this for?

This is need for the Timer used by the progress indicator. Added comment.

[sail, 2012-10-10 21:39:37]

http://codereview.chromium.org/11009017/diff/21021/content/shell/shell_browse...
File content/shell/shell_browser_main_parts_mac.mm (left):

http://codereview.chromium.org/11009017/diff/21021/content/shell/shell_browse...
content/shell/shell_browser_main_parts_mac.mm:5: #include
"content/shell/shell_browser_main_parts.h"
On 2012/10/10 21:29:36, rsesek wrote:
> ?

Bad patching. Fixed.

[Robert Sesek, 2012-10-10 21:40:58]

LGTM

http://codereview.chromium.org/11009017/diff/21021/chrome/browser/ui/cocoa/in...
File
chrome/browser/ui/cocoa/intents/web_intent_progress_view_controller_unittest.mm
(right):

http://codereview.chromium.org/11009017/diff/21021/chrome/browser/ui/cocoa/in...
chrome/browser/ui/cocoa/intents/web_intent_progress_view_controller_unittest.mm:23:
MessageLoop message_loop_;
On 2012/10/10 21:39:10, sail wrote:
> On 2012/10/10 21:29:36, rsesek wrote:
> > What's this for?
> 
> This is need for the Timer used by the progress indicator. Added comment.

Can you copy the comment to the other test, too?

[sail, 2012-10-10 21:47:41]

http://codereview.chromium.org/11009017/diff/21021/chrome/browser/ui/cocoa/in...
File
chrome/browser/ui/cocoa/intents/web_intent_progress_view_controller_unittest.mm
(right):

http://codereview.chromium.org/11009017/diff/21021/chrome/browser/ui/cocoa/in...
chrome/browser/ui/cocoa/intents/web_intent_progress_view_controller_unittest.mm:23:
MessageLoop message_loop_;
On 2012/10/10 21:40:58, rsesek wrote:
> On 2012/10/10 21:39:10, sail wrote:
> > On 2012/10/10 21:29:36, rsesek wrote:
> > > What's this for?
> > 
> > This is need for the Timer used by the progress indicator. Added comment.
> 
> Can you copy the comment to the other test, too?

Done.

[commit-bot: I haz the power, 2012-10-11 04:02:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/11009017/25003

[commit-bot: I haz the power, 2012-10-11 04:02:35]

Presubmit check for 11009017-25003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright (\(c\) )?(2012|2011|2010|2009|2008|2007|2006|2006-2008) The
Chromium Authors\. All rights reserved\.\n.*? Use of this source code is
governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?:
\*/)?\n
Found a bad license header in these files:
  chrome/browser/ui/cocoa/spinner_progress_indicator.mm

Presubmit checks took 1.4s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2012-10-11 04:04:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/11009017/17005

[commit-bot: I haz the power, 2012-10-11 05:52:52]

Change committed as 161302