Addressing UX regression on Win10 FRE, and changing timeout behavior

Addressing UX regression on Win10 FRE, and changing timeout behavior to
assume Chrome is not pinned to taskbar.

BUG=694118, 695614, 687042
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2722683002
Cr-Commit-Position: refs/heads/master@{#453484}
Committed: https://chromium.googlesource.com/chromium/src/+/2f2a6b849bcee8b531b45effa22de6ed03dc33d7

[tmartino, 2017-02-27 22:17:39]

Description was changed from

==========
Addressing UX regression on Win10 FRE, and changing timeout behavior to
assume Chrome is not pinned to taskbar.

BUG=694118, 695614
==========

to

==========
Addressing UX regression on Win10 FRE, and changing timeout behavior to
assume Chrome is not pinned to taskbar.

BUG=694118, 695614
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tmartino, 2017-02-27 22:17:50]

The CQ bit was checked by tmartino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-27 22:18:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tmartino, 2017-02-27 22:20:06]

Description was changed from

==========
Addressing UX regression on Win10 FRE, and changing timeout behavior to
assume Chrome is not pinned to taskbar.

BUG=694118, 695614
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Addressing UX regression on Win10 FRE, and changing timeout behavior to
assume Chrome is not pinned to taskbar.

BUG=694118, 695614, 687042
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tommycli, 2017-02-27 23:17:22]

tommycli@chromium.org changed reviewers:
+ tommycli@chromium.org

[tommycli, 2017-02-27 23:17:23]

https://codereview.chromium.org/2722683002/diff/1/chrome/browser/resources/we...
File chrome/browser/resources/welcome/win10/inline.css (right):

https://codereview.chromium.org/2722683002/diff/1/chrome/browser/resources/we...
chrome/browser/resources/welcome/win10/inline.css:18: cursor: pointer;
Hey, this shouldn't be necessary, since the action-link definitions should take
care of it:

https://cs.chromium.org/chromium/src/ui/webui/resources/css/action_link.css?q...

Looking on inspector, it looks like the action_link css isn't working on this
element.

This is how we normally apply imported styles in HTML imports:

https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/printi...

Also we generally prefer to inline styles these days rather than in a separate
file. (see above example)

Out of curiosity, how is inline.css even imported in? I don't see a reference to
it in inline.html

https://codereview.chromium.org/2722683002/diff/1/chrome/browser/resources/we...
File chrome/browser/resources/welcome/win10/inline.js (right):

https://codereview.chromium.org/2722683002/diff/1/chrome/browser/resources/we...
chrome/browser/resources/welcome/win10/inline.js:37: app.isCombined = true;
I guess I'm a bit confused as how changing this works? It appears that the value
is always overwritten when the 'getPinnedToTaskbarState' promise comes back..

[tmartino, 2017-02-28 00:08:53]

https://codereview.chromium.org/2722683002/diff/1/chrome/browser/resources/we...
File chrome/browser/resources/welcome/win10/inline.css (right):

https://codereview.chromium.org/2722683002/diff/1/chrome/browser/resources/we...
chrome/browser/resources/welcome/win10/inline.css:18: cursor: pointer;
On 2017/02/27 at 23:17:22, tommycli wrote:
> Hey, this shouldn't be necessary, since the action-link definitions should
take care of it:
> 
>
https://cs.chromium.org/chromium/src/ui/webui/resources/css/action_link.css?q...
> 
> Looking on inspector, it looks like the action_link css isn't working on this
element.
> 
> This is how we normally apply imported styles in HTML imports:
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/printi...
> 
> Also we generally prefer to inline styles these days rather than in a separate
file. (see above example)

Hmm, so I tried creating a <style> and found that it doesn't solve the cursor
issue. Interestingly it looks like the action_link JS is running--role=link is
getting added in all the right places--so it's just the css portion that's
broken. I suspect we should really change this whole page to a dom-module, but
I'm not familiar enough with Polymer to say for sure if that's at fault here.

I'm definitely happy to make that change and to inline the CSS--I'd prefer it
that way anyway--but in terms of a safe merge I'd be more comfortable making a
big restructuring change that way in a followup for M58. WDYT?

> Out of curiosity, how is inline.css even imported in? I don't see a reference
to it in inline.html

There are two layouts, inline.* and combined.*. Depending on a variations
feature, one of them is chosen by the UI class in C++land, and gets loaded as
welcome.*.

https://codereview.chromium.org/2722683002/diff/1/chrome/browser/resources/we...
File chrome/browser/resources/welcome/win10/inline.js (right):

https://codereview.chromium.org/2722683002/diff/1/chrome/browser/resources/we...
chrome/browser/resources/welcome/win10/inline.js:37: app.isCombined = true;
On 2017/02/27 at 23:17:22, tommycli wrote:
> I guess I'm a bit confused as how changing this works? It appears that the
value is always overwritten when the 'getPinnedToTaskbarState' promise comes
back..

Whoops, that's a great catch--this is slightly complicated by the fact that
timing out that system check isn't really something you can intentionally repro
when testing.

The correct bool change is made in the cc file I've just uploaded.

[tommycli, 2017-02-28 01:06:28]

lgtm thanks!

https://codereview.chromium.org/2722683002/diff/1/chrome/browser/resources/we...
File chrome/browser/resources/welcome/win10/inline.css (right):

https://codereview.chromium.org/2722683002/diff/1/chrome/browser/resources/we...
chrome/browser/resources/welcome/win10/inline.css:18: cursor: pointer;
On 2017/02/28 00:08:53, tmartino wrote:
> On 2017/02/27 at 23:17:22, tommycli wrote:
> > Hey, this shouldn't be necessary, since the action-link definitions should
> take care of it:
> > 
> >
>
https://cs.chromium.org/chromium/src/ui/webui/resources/css/action_link.css?q...
> > 
> > Looking on inspector, it looks like the action_link css isn't working on
this
> element.
> > 
> > This is how we normally apply imported styles in HTML imports:
> > 
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/printi...
> > 
> > Also we generally prefer to inline styles these days rather than in a
separate
> file. (see above example)
> 
> Hmm, so I tried creating a <style> and found that it doesn't solve the cursor
> issue. Interestingly it looks like the action_link JS is running--role=link is
> getting added in all the right places--so it's just the css portion that's
> broken. I suspect we should really change this whole page to a dom-module, but
> I'm not familiar enough with Polymer to say for sure if that's at fault here.
> 
> I'm definitely happy to make that change and to inline the CSS--I'd prefer it
> that way anyway--but in terms of a safe merge I'd be more comfortable making a
> big restructuring change that way in a followup for M58. WDYT?
> 
> > Out of curiosity, how is inline.css even imported in? I don't see a
reference
> to it in inline.html
> 
> There are two layouts, inline.* and combined.*. Depending on a variations
> feature, one of them is chosen by the UI class in C++land, and gets loaded as
> welcome.*.

I'm okay with fixing this up in a followup. Do you mind adding a TODO?

[tmartino, 2017-02-28 01:51:21]

The CQ bit was checked by tmartino@chromium.org

[tmartino, 2017-02-28 01:51:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/2722683002/#ps60001
(title: "adding todo")

[commit-bot: I haz the power, 2017-02-28 01:51:51]

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, 2017-02-28 03:39:07]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488246681512710,
"parent_rev": "75d4f942efa44275b81da51f5cffe0313bd5e600", "commit_rev":
"2f2a6b849bcee8b531b45effa22de6ed03dc33d7"}

[commit-bot: I haz the power, 2017-02-28 03:39:46]

Description was changed from

==========
Addressing UX regression on Win10 FRE, and changing timeout behavior to
assume Chrome is not pinned to taskbar.

BUG=694118, 695614, 687042
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Addressing UX regression on Win10 FRE, and changing timeout behavior to
assume Chrome is not pinned to taskbar.

BUG=694118, 695614, 687042
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2722683002
Cr-Commit-Position: refs/heads/master@{#453484}
Committed:
https://chromium.googlesource.com/chromium/src/+/2f2a6b849bcee8b531b45effa22d...
==========

[commit-bot: I haz the power, 2017-02-28 03:39:47]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/2f2a6b849bcee8b531b45effa22d...