Creates GCP promo for ChromeOS users with no cloud printers.

Creates GCP promo for ChromeOS users with no cloud printers.

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

[ydolgov1, 2012-10-16 21:44:54]

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/app_state.js (right):

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/app_state.js:81:
this.isGcpPromoDismissed_ = true;
Why is it true by default? Should we initialize it with null as we do for other
boolean fields?

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/app_state.js:81:
this.isGcpPromoDismissed_ = true;
And also can you move it closer to selectedDestinationId_ just to keep all the
global application properties together.

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/no_destinations_promo.css (right):

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/no_destinations_promo.css:12: width:
640px;
Just curious why does it have fixed width?

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/no_destinations_promo.html (right):

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/no_destinations_promo.html:1: <div
id="no-destinations-promo" class="overlay" hidden>
Don't you have comments for templates in Chrome?

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/previewarea/preview_area.js (left):

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/previewarea/preview_area.js:154:
PDF_PLUGIN: 'preview-area-pdf-plugin'
Not sure I understand why you removed this class from the enum and started to
use string instead?

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/print_preview.js (right):

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/print_preview.js:323: this.tracker.add(
Why don't you wanna create CloudPrintPromo class that would render template into
an element that user of the class provides and would collect all the clicks and
persist it's state?

[Toscano, 2012-10-16 22:24:14]

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/app_state.js (right):

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/app_state.js:81:
this.isGcpPromoDismissed_ = true;
I'd like to keep the default behavior to not show the promo. This actually
solves a small bug where the the cloud print requests finish before the AppState
is initialized resulting in the promo flashing on and off for a split second.

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/app_state.js:81:
this.isGcpPromoDismissed_ = true;
On 2012/10/16 21:44:54, ydolgov1 wrote:
> And also can you move it closer to selectedDestinationId_ just to keep all the
> global application properties together.

Done.

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/no_destinations_promo.css (right):

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/no_destinations_promo.css:12: width:
640px;
On 2012/10/16 21:44:54, ydolgov1 wrote:
> Just curious why does it have fixed width?

Part of the design spec for dialogs in Chrome web-ui.

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/no_destinations_promo.html (right):

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/no_destinations_promo.html:1: <div
id="no-destinations-promo" class="overlay" hidden>
On 2012/10/16 21:44:54, ydolgov1 wrote:
> Don't you have comments for templates in Chrome?

Nope. Would be nice if the flattener would strip comments. But no support yet.

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/previewarea/preview_area.js (left):

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/previewarea/preview_area.js:154:
PDF_PLUGIN: 'preview-area-pdf-plugin'
On 2012/10/16 21:44:54, ydolgov1 wrote:
> Not sure I understand why you removed this class from the enum and started to
> use string instead?

This class was no longer used. So I removed it. But I'd like to get rid of these
Classes_ enums and migrate to just using concise strings.

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/print_preview.js (right):

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/print_preview.js:323: this.tracker.add(
On 2012/10/16 21:44:54, ydolgov1 wrote:
> Why don't you wanna create CloudPrintPromo class that would render template
into
> an element that user of the class provides and would collect all the clicks
and
> persist it's state?

Tempting. The first iteration wasn't very complex so it was fine including it in
print_preview.js, but now it has grown a little more complex.

I think it makes sense to move it into separate class, but in a later CL. Before
I do that, I want to abstract some common behavior for all print preview dialogs
(e.g. Destination Search dialog) into a common base class, and have the promo
widget inherit from that.

[ydolgov1, 2012-10-16 22:43:17]

lgtm

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/app_state.js (right):

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/app_state.js:166: /** @return
{boolean} Whether the GCP promotion has been dismissed. */
Can you also move this one closer to isSelectedDestinationLocal?

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/app_state.js:215:
this.isGcpPromoDismissed_ =
And this closer to "this.selectedDestinationId_ =..."?

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/app_state.js:299:
persistIsGcpPromoDismissed: function(isGcpPromoDismissed) {
Please fix indentation.

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/app_state.js:325:
obj[AppState.Field_.IS_GCP_PROMO_DISMISSED] = this.isGcpPromoDismissed_;
And also please move this one closer to
"obj[AppState.Field_.SELECTED_DESTINATION_ID]..."

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/previewarea/preview_area.js (right):

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/previewarea/preview_area.js:420:
this.getChildElement('.preview-area-plugin-wrapper').
Do you have any rule when you use getChildElement and when you use
getElementsByClassName?

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/print_preview.js (right):

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/print_preview.js:323: this.tracker.add(
O.k. sounds good, can you add a TODO for now?

On 2012/10/16 22:24:14, Toscano wrote:
> On 2012/10/16 21:44:54, ydolgov1 wrote:
> > Why don't you wanna create CloudPrintPromo class that would render template
> into
> > an element that user of the class provides and would collect all the clicks
> and
> > persist it's state?
> 
> Tempting. The first iteration wasn't very complex so it was fine including it
in
> print_preview.js, but now it has grown a little more complex.
> 
> I think it makes sense to move it into separate class, but in a later CL.
Before
> I do that, I want to abstract some common behavior for all print preview
dialogs
> (e.g. Destination Search dialog) into a common base class, and have the promo
> widget inherit from that.

[Toscano, 2012-10-16 22:58:44]

gene@, can you take a look at the C++ side of things?

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/app_state.js (right):

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/app_state.js:166: /** @return
{boolean} Whether the GCP promotion has been dismissed. */
On 2012/10/16 22:43:17, ydolgov1 wrote:
> Can you also move this one closer to isSelectedDestinationLocal?

Done.

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/app_state.js:215:
this.isGcpPromoDismissed_ =
On 2012/10/16 22:43:17, ydolgov1 wrote:
> And this closer to "this.selectedDestinationId_ =..."?

Done.

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/app_state.js:299:
persistIsGcpPromoDismissed: function(isGcpPromoDismissed) {
On 2012/10/16 22:43:17, ydolgov1 wrote:
> Please fix indentation.

Done.

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/app_state.js:325:
obj[AppState.Field_.IS_GCP_PROMO_DISMISSED] = this.isGcpPromoDismissed_;
On 2012/10/16 22:43:17, ydolgov1 wrote:
> And also please move this one closer to
> "obj[AppState.Field_.SELECTED_DESTINATION_ID]..."

Done.

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/previewarea/preview_area.js (right):

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/previewarea/preview_area.js:420:
this.getChildElement('.preview-area-plugin-wrapper').
On 2012/10/16 22:43:17, ydolgov1 wrote:
> Do you have any rule when you use getChildElement and when you use
> getElementsByClassName?

Yes. Always use getChildElement. I want to move away from the
getElementsByClassName method. Some classes have not yet been migrated.

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/print_preview.js (right):

https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/print_preview.js:323: this.tracker.add(
On 2012/10/16 22:43:17, ydolgov1 wrote:
> O.k. sounds good, can you add a TODO for now?
> 
> On 2012/10/16 22:24:14, Toscano wrote:
> > On 2012/10/16 21:44:54, ydolgov1 wrote:
> > > Why don't you wanna create CloudPrintPromo class that would render
template
> > into
> > > an element that user of the class provides and would collect all the
clicks
> > and
> > > persist it's state?
> > 
> > Tempting. The first iteration wasn't very complex so it was fine including
it
> in
> > print_preview.js, but now it has grown a little more complex.
> > 
> > I think it makes sense to move it into separate class, but in a later CL.
> Before
> > I do that, I want to abstract some common behavior for all print preview
> dialogs
> > (e.g. Destination Search dialog) into a common base class, and have the
promo
> > widget inherit from that.
> 

Done.

[gene, 2012-10-16 23:16:38]

lgtm

On 2012/10/16 22:58:44, Toscano wrote:
> gene@, can you take a look at the C++ side of things?
> 
>
https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
> File chrome/browser/resources/print_preview/data/app_state.js (right):
> 
>
https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
> chrome/browser/resources/print_preview/data/app_state.js:166: /** @return
> {boolean} Whether the GCP promotion has been dismissed. */
> On 2012/10/16 22:43:17, ydolgov1 wrote:
> > Can you also move this one closer to isSelectedDestinationLocal?
> 
> Done.
> 
>
https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
> chrome/browser/resources/print_preview/data/app_state.js:215:
> this.isGcpPromoDismissed_ =
> On 2012/10/16 22:43:17, ydolgov1 wrote:
> > And this closer to "this.selectedDestinationId_ =..."?
> 
> Done.
> 
>
https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
> chrome/browser/resources/print_preview/data/app_state.js:299:
> persistIsGcpPromoDismissed: function(isGcpPromoDismissed) {
> On 2012/10/16 22:43:17, ydolgov1 wrote:
> > Please fix indentation.
> 
> Done.
> 
>
https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
> chrome/browser/resources/print_preview/data/app_state.js:325:
> obj[AppState.Field_.IS_GCP_PROMO_DISMISSED] = this.isGcpPromoDismissed_;
> On 2012/10/16 22:43:17, ydolgov1 wrote:
> > And also please move this one closer to
> > "obj[AppState.Field_.SELECTED_DESTINATION_ID]..."
> 
> Done.
> 
>
https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
> File chrome/browser/resources/print_preview/previewarea/preview_area.js
(right):
> 
>
https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
> chrome/browser/resources/print_preview/previewarea/preview_area.js:420:
> this.getChildElement('.preview-area-plugin-wrapper').
> On 2012/10/16 22:43:17, ydolgov1 wrote:
> > Do you have any rule when you use getChildElement and when you use
> > getElementsByClassName?
> 
> Yes. Always use getChildElement. I want to move away from the
> getElementsByClassName method. Some classes have not yet been migrated.
> 
>
https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
> File chrome/browser/resources/print_preview/print_preview.js (right):
> 
>
https://codereview.chromium.org/11194020/diff/3001/chrome/browser/resources/p...
> chrome/browser/resources/print_preview/print_preview.js:323: this.tracker.add(
> On 2012/10/16 22:43:17, ydolgov1 wrote:
> > O.k. sounds good, can you add a TODO for now?
> > 
> > On 2012/10/16 22:24:14, Toscano wrote:
> > > On 2012/10/16 21:44:54, ydolgov1 wrote:
> > > > Why don't you wanna create CloudPrintPromo class that would render
> template
> > > into
> > > > an element that user of the class provides and would collect all the
> clicks
> > > and
> > > > persist it's state?
> > > 
> > > Tempting. The first iteration wasn't very complex so it was fine including
> it
> > in
> > > print_preview.js, but now it has grown a little more complex.
> > > 
> > > I think it makes sense to move it into separate class, but in a later CL.
> > Before
> > > I do that, I want to abstract some common behavior for all print preview
> > dialogs
> > > (e.g. Destination Search dialog) into a common base class, and have the
> promo
> > > widget inherit from that.
> > 
> 
> Done.

[commit-bot: I haz the power, 2012-10-17 19:46:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rltoscano@chromium.org/11194020/1017

[commit-bot: I haz the power, 2012-10-17 21:42:22]

Change committed as 162538