[Local NTP] Cleanup: Don't create HTML elements dynamically

[Local NTP] Cleanup: Don't create HTML elements dynamically

Before this CL, many of the HTML elements on the local NTP were created
dynamically in JS, for no good reason. This CL moves the element
definitions into HTML where they belong.

BUG=687972
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2684593002
Cr-Commit-Position: refs/heads/master@{#449001}
Committed: https://chromium.googlesource.com/chromium/src/+/5af7397e1d8ae4d18d03bbfc174227fc57d6a15c

[Marc Treib, 2017-02-07 13:59:12]

Description was changed from

==========
[Local NTP] Cleanup: Don't create HTML elements dynamically

Before this CL, many of the HTML elements on the local NTP were created
dynamically in JS, for no good reason. This CL moves the element
definitions into HTML where they belong.

BUG=687972
==========

to

==========
[Local NTP] Cleanup: Don't create HTML elements dynamically

Before this CL, many of the HTML elements on the local NTP were created
dynamically in JS, for no good reason. This CL moves the element
definitions into HTML where they belong.

BUG=687972
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Marc Treib, 2017-02-07 14:27:01]

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

[commit-bot: I haz the power, 2017-02-07 14:27:16]

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

[Marc Treib, 2017-02-07 14:37:11]

treib@chromium.org changed reviewers:
+ sfiera@chromium.org

[Marc Treib, 2017-02-07 14:37:12]

PTAL!

https://codereview.chromium.org/2684593002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/local_ntp/local_ntp.js (left):

https://codereview.chromium.org/2684593002/diff/20001/chrome/browser/resource...
chrome/browser/resources/local_ntp/local_ntp.js:254: }
This block moved into init() where it belongs - it has nothing to do with the
theme.
Bonus: Before, the "Search Google or type URL" string would flicker on reload,
because it was set after the initial page load. Now it's all better.

https://codereview.chromium.org/2684593002/diff/20001/chrome/test/data/local_...
File chrome/test/data/local_ntp_browsertest.js (right):

https://codereview.chromium.org/2684593002/diff/20001/chrome/test/data/local_...
chrome/test/data/local_ntp_browsertest.js:98:
assert(!elementIsVisible($('logo')));
Before, the elements didn't exist at all; now they do exist but have "display:
none". IMO that's perfectly fine.

[sfiera, 2017-02-07 14:45:26]

LGTM

[Marc Treib, 2017-02-07 15:40:24]

The CQ bit was unchecked by treib@chromium.org

[Marc Treib, 2017-02-07 15:40:24]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2017-02-07 15:40:31]

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-07 16:37:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 16:37:15]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Marc Treib, 2017-02-08 13:41:33]

On 2017/02/07 16:37:15, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Huh, turns out the test failures are real, though I sure don't see how they're
my fault. Here's what happens:

The MV iframe posts a message to its parent, to an origin that's set from
IframeSource::GetOrigin. With browser-side-navigation enabled, the origin is
somehow determined to be some 127.0.0.1 URL (pointing to the embedded test
server that those browser tests use), but *only* if the iframe element is
defined in the html. If it's created dynamically in js, then the origin is
determined as chrome-search://local-ntp as it should and all is well.

Still no real idea why that happens - I'm guessing somehow the exact order of
events matters, and in the non-dynamic iframe + browser-side-navigation case,
somehow the navigation away from the NTP happens before IframeSource::GetOrigin
gets run?

For now, I'll just go back to creating the iframe dynamically. Hopefully at some
point there'll be time to investigate further (haha), or, better yet, the
browser-side-navigation folks fix this :D

[Marc Treib, 2017-02-08 13:56:55]

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

[commit-bot: I haz the power, 2017-02-08 13:57:06]

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

[sfiera, 2017-02-08 14:03:52]

Hmm. Well, isn't the HTML on 127.0.0.1 and the Javascript in
chrome-search://local-ntp then? Makes sense that the origin would be different
based on which file it's from.

[Marc Treib, 2017-02-08 14:08:30]

On 2017/02/08 14:03:52, sfiera wrote:
> Hmm. Well, isn't the HTML on 127.0.0.1 and the Javascript in
> chrome-search://local-ntp then? Makes sense that the origin would be different
> based on which file it's from.

No, the HTML is still on chrome-search://local-ntp either way, but the
non-dynamically-created iframe tries to postMessage to 127.0.0.1, hence the JS
error that makes those tests fail.
The weird thing is that IframeSource::GetOrigin gives a different result
depending on whether the iframe is created dynamically or not.

[commit-bot: I haz the power, 2017-02-08 15:35:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 15:35:11]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2017-02-08 15:35:29]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2017-02-08 15:35:30]

The patchset sent to the CQ was uploaded after l-g-t-m from sfiera@chromium.org
Link to the patchset: https://codereview.chromium.org/2684593002/#ps40001
(title: "go back to dynamically creating the iframe")

[commit-bot: I haz the power, 2017-02-08 15:35:54]

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-08 15:38:56]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486568129654350,
"parent_rev": "e76f62c07f2dfbddef18d4011c576f678fc1f54b", "commit_rev":
"5af7397e1d8ae4d18d03bbfc174227fc57d6a15c"}

[commit-bot: I haz the power, 2017-02-08 15:39:50]

Description was changed from

==========
[Local NTP] Cleanup: Don't create HTML elements dynamically

Before this CL, many of the HTML elements on the local NTP were created
dynamically in JS, for no good reason. This CL moves the element
definitions into HTML where they belong.

BUG=687972
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[Local NTP] Cleanup: Don't create HTML elements dynamically

Before this CL, many of the HTML elements on the local NTP were created
dynamically in JS, for no good reason. This CL moves the element
definitions into HTML where they belong.

BUG=687972
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2684593002
Cr-Commit-Position: refs/heads/master@{#449001}
Committed:
https://chromium.googlesource.com/chromium/src/+/5af7397e1d8ae4d18d03bbfc1742...
==========

[commit-bot: I haz the power, 2017-02-08 15:39:51]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/5af7397e1d8ae4d18d03bbfc1742...