Issue 10896032: HTML titlebars for v2 apps.

Issue 10896032: HTML titlebars for v2 apps. (Closed)

Created:
8 years, 3 months ago by jeremya

Modified:
8 years, 3 months ago

Reviewers:
koz (OOO until 15th September), Mihai Parparita -not on Chrome, James Hawkins, abarth-chromium

CC:
chromium-reviews, jeremya+watch_chromium.org, Aaron Boodman, mihaip-chromium-reviews_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org, benwells, jhawkins

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

HTML titlebars for v2 apps. All shell windows are now frameless windows, and the FRAME_CHROME option will be removed presently. In the case that the developer specifies {frame:'chrome'} (or no frame: option at all) to the chrome.app.window.create function, we inject a standard titlebar and controls into the shadow root of the <html> element when the document is created. This is so that the titlebar can be rendered immediately, without waiting for the onload event, which could be delivered long after the window is shown. TBR=jhawkins@chromium.org BUG=144902 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154904

Patch Set 1 #

Total comments: 14

Patch Set 2 : comments #

Patch Set 3 : move function call logic into ModuleSystem #

Total comments: 10

Patch Set 4 : fixes #

Total comments: 2

Patch Set 5 : . #

Patch Set 6 : amend ChromeRenderViewObserver #

Patch Set 7 : copyright #

Patch Set 8 : kludge test fix for mac #

Created: 8 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+142 lines, -25 lines)			Patch
M	chrome/browser/extensions/api/app_window/app_window_api.cc	View	1 2 3 4	1 chunk	+0 lines, -6 lines	0 comments	Download
M	chrome/browser/ui/extensions/shell_window.cc	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/renderer/chrome_render_view_observer.cc	View	1 2 3 4 5	1 chunk	+4 lines, -0 lines	0 comments	Download
M	chrome/renderer/extensions/app_window_custom_bindings.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/renderer/extensions/app_window_custom_bindings.cc	View	1 2 3	3 chunks	+41 lines, -14 lines	0 comments	Download
M	chrome/renderer/extensions/chrome_v8_context.h	View	1 2 3 4	1 chunk	+2 lines, -0 lines	0 comments	Download
M	chrome/renderer/extensions/dispatcher.cc	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/renderer/extensions/module_system.h	View	1 2 3 4	1 chunk	+5 lines, -0 lines	0 comments	Download
M	chrome/renderer/extensions/module_system.cc	View	1 2 3 4	1 chunk	+27 lines, -0 lines	0 comments	Download
M	chrome/renderer/renderer_resources.grd	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/renderer/resources/extensions/app_window_custom_bindings.js	View	1 2 3 4	1 chunk	+4 lines, -2 lines	0 comments	Download
A	chrome/renderer/resources/extensions/inject_app_titlebar.js	View	1 2 3 4 5 6	1 chunk	+48 lines, -0 lines	0 comments	Download
M	chrome/renderer/resources/extensions/platform_app.css	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/test/data/extensions/platform_apps/windows_api/test.js	View	1 2 3 4 5 6 7	1 chunk	+6 lines, -1 line	0 comments	Download

Messages

Total messages: 32 (0 generated)

Expand Messages | Collapse Messages

jeremya

Preliminary implementation -- not sure who the best reviewer is for this. Please let me ...

8 years, 3 months ago (2012-08-29 05:43:26 UTC) #1

abarth-chromium

Looks more or less reasonable to me. I'm not sure who your best reviewer is, ...

8 years, 3 months ago (2012-08-29 23:51:20 UTC) #2

Mihai Parparita -not on Chrome

https://chromiumcodereview.appspot.com/10896032/diff/1/chrome/renderer/extensions/app_window_custom_bindings.cc File chrome/renderer/extensions/app_window_custom_bindings.cc (right): https://chromiumcodereview.appspot.com/10896032/diff/1/chrome/renderer/extensions/app_window_custom_bindings.cc#newcode39 chrome/renderer/extensions/app_window_custom_bindings.cc:39: virtual void DidCreateDocumentElement(WebKit::WebFrame* frame) { Nit: OVERRIDE https://chromiumcodereview.appspot.com/10896032/diff/1/chrome/renderer/resources/extensions/inject_app_titlebar.js File ...

8 years, 3 months ago (2012-08-30 01:13:47 UTC) #3

jeremya

http://codereview.chromium.org/10896032/diff/1/chrome/renderer/resources/extensions/inject_app_titlebar.js File chrome/renderer/resources/extensions/inject_app_titlebar.js (right): http://codereview.chromium.org/10896032/diff/1/chrome/renderer/resources/extensions/inject_app_titlebar.js#newcode25 chrome/renderer/resources/extensions/inject_app_titlebar.js:25: document.onreadystatechange = function() { On 2012/08/30 01:13:47, Mihai Parparita ...

8 years, 3 months ago (2012-08-30 01:45:18 UTC) #4

jeremya

8 years, 3 months ago (2012-08-30 01:50:06 UTC) #5

jeremya

http://codereview.chromium.org/10896032/diff/1/chrome/renderer/extensions/app_window_custom_bindings.cc File chrome/renderer/extensions/app_window_custom_bindings.cc (right): http://codereview.chromium.org/10896032/diff/1/chrome/renderer/extensions/app_window_custom_bindings.cc#newcode39 chrome/renderer/extensions/app_window_custom_bindings.cc:39: virtual void DidCreateDocumentElement(WebKit::WebFrame* frame) { On 2012/08/30 01:13:47, Mihai ...

8 years, 3 months ago (2012-08-30 02:21:01 UTC) #6

Mihai Parparita -not on Chrome

On Wed, Aug 29, 2012 at 6:50 PM, <jeremya@chromium.org> wrote: > (and also: readystatechange doesn't ...

8 years, 3 months ago (2012-08-30 23:05:40 UTC) #7

Mihai Parparita -not on Chrome

On Wed, Aug 29, 2012 at 6:50 PM, <jeremya@chromium.org> wrote: > (and also: readystatechange doesn't ...

8 years, 3 months ago (2012-08-30 23:05:40 UTC) #8

abarth-chromium

On Thu, Aug 30, 2012 at 4:05 PM, Mihai Parparita <mihaip@chromium.org> wrote: > On Wed, ...

8 years, 3 months ago (2012-08-31 00:23:27 UTC) #9

abarth-chromium

On Thu, Aug 30, 2012 at 4:05 PM, Mihai Parparita <mihaip@chromium.org> wrote: > On Wed, ...

8 years, 3 months ago (2012-08-31 00:23:27 UTC) #10

jeremya

platform_app.js should be run at a fairly normal time (along with all the other extension ...

8 years, 3 months ago (2012-08-31 00:25:39 UTC) #11

platform_app.js should be run at a fairly normal time (along with all the
other extension JS that gets run when a page is a created).
inject_app_titlebar.js is the weird one, and it looks like the
onreadystatechange event with capture fires at the right time. So I should
be able to remove the DidCreateDocumentElement listener from the C++ :)


On Fri, Aug 31, 2012 at 10:23 AM, Adam Barth <abarth@chromium.org> wrote:

> On Thu, Aug 30, 2012 at 4:05 PM, Mihai Parparita <mihaip@chromium.org>
> wrote:
> > On Wed, Aug 29, 2012 at 6:50 PM, <jeremya@chromium.org> wrote:
> >>
> >> (and also: readystatechange doesn't bubble to window, so I can't add a
> >> listener on the window. The problem here is that the document element
> >> gets substituted out when the navigation happens, so any event listeners
> >> I register on the document at window creation time are gone by the time
> >> the second document gets loaded.)
> >
> >
> > You don't actually need to to bubble to the window, you can listen for
> it at
> > the window level in the capturing phase. Something like
> >
> > window.addEventListener('readystatechange', function() { ...}, true /*
> true
> > == capture */);
> >
> > I experimented with that in in platform_app.js, and as far as I can tell,
> > while platform_app.js is being run, document points to the old document
> (as
> > you said). However, if I add an event listener as above to the window, it
> > will fire (with the document in event.target) with the new document
> before
> > the page has had any chance to run its code, thus it seems like a good
> time
> > to do the titlebar injection.
>
> I'm concerned that we're running platform_app.js at a super strange
> time in the middle of navigation.  I mentioned this before, but I'm
> worried that the more complexity we pile into platform_app.js the more
> fragile it will become and the harder it will be to fix it later.
>
> Can we provide you better APIs to call you back at exactly the right
> time so you don't need to resort to this sort of hackery?
>
> Adam
>

jeremya

platform_app.js should be run at a fairly normal time (along with all the other extension ...

8 years, 3 months ago (2012-08-31 00:25:40 UTC) #12

platform_app.js should be run at a fairly normal time (along with all the
other extension JS that gets run when a page is a created).
inject_app_titlebar.js is the weird one, and it looks like the
onreadystatechange event with capture fires at the right time. So I should
be able to remove the DidCreateDocumentElement listener from the C++ :)


On Fri, Aug 31, 2012 at 10:23 AM, Adam Barth <abarth@chromium.org> wrote:

> On Thu, Aug 30, 2012 at 4:05 PM, Mihai Parparita <mihaip@chromium.org>
> wrote:
> > On Wed, Aug 29, 2012 at 6:50 PM, <jeremya@chromium.org> wrote:
> >>
> >> (and also: readystatechange doesn't bubble to window, so I can't add a
> >> listener on the window. The problem here is that the document element
> >> gets substituted out when the navigation happens, so any event listeners
> >> I register on the document at window creation time are gone by the time
> >> the second document gets loaded.)
> >
> >
> > You don't actually need to to bubble to the window, you can listen for
> it at
> > the window level in the capturing phase. Something like
> >
> > window.addEventListener('readystatechange', function() { ...}, true /*
> true
> > == capture */);
> >
> > I experimented with that in in platform_app.js, and as far as I can tell,
> > while platform_app.js is being run, document points to the old document
> (as
> > you said). However, if I add an event listener as above to the window, it
> > will fire (with the document in event.target) with the new document
> before
> > the page has had any chance to run its code, thus it seems like a good
> time
> > to do the titlebar injection.
>
> I'm concerned that we're running platform_app.js at a super strange
> time in the middle of navigation.  I mentioned this before, but I'm
> worried that the more complexity we pile into platform_app.js the more
> fragile it will become and the harder it will be to fix it later.
>
> Can we provide you better APIs to call you back at exactly the right
> time so you don't need to resort to this sort of hackery?
>
> Adam
>

jeremya

... on second thoughts, it looks like the onreadystatechange event gets fired at an even ...

8 years, 3 months ago (2012-08-31 00:34:24 UTC) #13

... on second thoughts, it looks like the onreadystatechange event gets
fired at an even weirder time:
window.addEventListener('readystatechange', function() {
  if (document.readyState == 'loading') {
    assert(document.childNodes.length == 0);
    assert(document.innerHTML == undefined);
  }
});
... i.e, even the <html> element hasn't been created yet!


On Fri, Aug 31, 2012 at 10:25 AM, Jeremy Apthorp <jeremya@chromium.org>wrote:

> platform_app.js should be run at a fairly normal time (along with all the
> other extension JS that gets run when a page is a created).
> inject_app_titlebar.js is the weird one, and it looks like the
> onreadystatechange event with capture fires at the right time. So I should
> be able to remove the DidCreateDocumentElement listener from the C++ :)
>
>
> On Fri, Aug 31, 2012 at 10:23 AM, Adam Barth <abarth@chromium.org> wrote:
>
>> On Thu, Aug 30, 2012 at 4:05 PM, Mihai Parparita <mihaip@chromium.org>
>> wrote:
>> > On Wed, Aug 29, 2012 at 6:50 PM, <jeremya@chromium.org> wrote:
>> >>
>> >> (and also: readystatechange doesn't bubble to window, so I can't add a
>> >> listener on the window. The problem here is that the document element
>> >> gets substituted out when the navigation happens, so any event
>> listeners
>> >> I register on the document at window creation time are gone by the time
>> >> the second document gets loaded.)
>> >
>> >
>> > You don't actually need to to bubble to the window, you can listen for
>> it at
>> > the window level in the capturing phase. Something like
>> >
>> > window.addEventListener('readystatechange', function() { ...}, true /*
>> true
>> > == capture */);
>> >
>> > I experimented with that in in platform_app.js, and as far as I can
>> tell,
>> > while platform_app.js is being run, document points to the old document
>> (as
>> > you said). However, if I add an event listener as above to the window,
>> it
>> > will fire (with the document in event.target) with the new document
>> before
>> > the page has had any chance to run its code, thus it seems like a good
>> time
>> > to do the titlebar injection.
>>
>> I'm concerned that we're running platform_app.js at a super strange
>> time in the middle of navigation.  I mentioned this before, but I'm
>> worried that the more complexity we pile into platform_app.js the more
>> fragile it will become and the harder it will be to fix it later.
>>
>> Can we provide you better APIs to call you back at exactly the right
>> time so you don't need to resort to this sort of hackery?
>>
>> Adam
>>
>
>

jeremya

... on second thoughts, it looks like the onreadystatechange event gets fired at an even ...

8 years, 3 months ago (2012-08-31 00:34:24 UTC) #14

... on second thoughts, it looks like the onreadystatechange event gets
fired at an even weirder time:
window.addEventListener('readystatechange', function() {
  if (document.readyState == 'loading') {
    assert(document.childNodes.length == 0);
    assert(document.innerHTML == undefined);
  }
});
... i.e, even the <html> element hasn't been created yet!


On Fri, Aug 31, 2012 at 10:25 AM, Jeremy Apthorp <jeremya@chromium.org>wrote:

> platform_app.js should be run at a fairly normal time (along with all the
> other extension JS that gets run when a page is a created).
> inject_app_titlebar.js is the weird one, and it looks like the
> onreadystatechange event with capture fires at the right time. So I should
> be able to remove the DidCreateDocumentElement listener from the C++ :)
>
>
> On Fri, Aug 31, 2012 at 10:23 AM, Adam Barth <abarth@chromium.org> wrote:
>
>> On Thu, Aug 30, 2012 at 4:05 PM, Mihai Parparita <mihaip@chromium.org>
>> wrote:
>> > On Wed, Aug 29, 2012 at 6:50 PM, <jeremya@chromium.org> wrote:
>> >>
>> >> (and also: readystatechange doesn't bubble to window, so I can't add a
>> >> listener on the window. The problem here is that the document element
>> >> gets substituted out when the navigation happens, so any event
>> listeners
>> >> I register on the document at window creation time are gone by the time
>> >> the second document gets loaded.)
>> >
>> >
>> > You don't actually need to to bubble to the window, you can listen for
>> it at
>> > the window level in the capturing phase. Something like
>> >
>> > window.addEventListener('readystatechange', function() { ...}, true /*
>> true
>> > == capture */);
>> >
>> > I experimented with that in in platform_app.js, and as far as I can
>> tell,
>> > while platform_app.js is being run, document points to the old document
>> (as
>> > you said). However, if I add an event listener as above to the window,
>> it
>> > will fire (with the document in event.target) with the new document
>> before
>> > the page has had any chance to run its code, thus it seems like a good
>> time
>> > to do the titlebar injection.
>>
>> I'm concerned that we're running platform_app.js at a super strange
>> time in the middle of navigation.  I mentioned this before, but I'm
>> worried that the more complexity we pile into platform_app.js the more
>> fragile it will become and the harder it will be to fix it later.
>>
>> Can we provide you better APIs to call you back at exactly the right
>> time so you don't need to resort to this sort of hackery?
>>
>> Adam
>>
>
>

abarth-chromium

platform_app.js is definitely not run at a "normal" time unless it has changed since the ...

8 years, 3 months ago (2012-08-31 00:37:12 UTC) #15

platform_app.js is definitely not run at a "normal" time unless it has
changed since the recent spat of platform_app.js-related crashes.

Adam


On Thu, Aug 30, 2012 at 5:25 PM, Jeremy Apthorp <jeremya@chromium.org> wrote:
> platform_app.js should be run at a fairly normal time (along with all the
> other extension JS that gets run when a page is a created).
> inject_app_titlebar.js is the weird one, and it looks like the
> onreadystatechange event with capture fires at the right time. So I should
> be able to remove the DidCreateDocumentElement listener from the C++ :)
>
>
> On Fri, Aug 31, 2012 at 10:23 AM, Adam Barth <abarth@chromium.org> wrote:
>>
>> On Thu, Aug 30, 2012 at 4:05 PM, Mihai Parparita <mihaip@chromium.org>
>> wrote:
>> > On Wed, Aug 29, 2012 at 6:50 PM, <jeremya@chromium.org> wrote:
>> >>
>> >> (and also: readystatechange doesn't bubble to window, so I can't add a
>> >> listener on the window. The problem here is that the document element
>> >> gets substituted out when the navigation happens, so any event
>> >> listeners
>> >> I register on the document at window creation time are gone by the time
>> >> the second document gets loaded.)
>> >
>> >
>> > You don't actually need to to bubble to the window, you can listen for
>> > it at
>> > the window level in the capturing phase. Something like
>> >
>> > window.addEventListener('readystatechange', function() { ...}, true /*
>> > true
>> > == capture */);
>> >
>> > I experimented with that in in platform_app.js, and as far as I can
>> > tell,
>> > while platform_app.js is being run, document points to the old document
>> > (as
>> > you said). However, if I add an event listener as above to the window,
>> > it
>> > will fire (with the document in event.target) with the new document
>> > before
>> > the page has had any chance to run its code, thus it seems like a good
>> > time
>> > to do the titlebar injection.
>>
>> I'm concerned that we're running platform_app.js at a super strange
>> time in the middle of navigation.  I mentioned this before, but I'm
>> worried that the more complexity we pile into platform_app.js the more
>> fragile it will become and the harder it will be to fix it later.
>>
>> Can we provide you better APIs to call you back at exactly the right
>> time so you don't need to resort to this sort of hackery?
>>
>> Adam
>
>

abarth-chromium

platform_app.js is definitely not run at a "normal" time unless it has changed since the ...

8 years, 3 months ago (2012-08-31 00:37:12 UTC) #16

platform_app.js is definitely not run at a "normal" time unless it has
changed since the recent spat of platform_app.js-related crashes.

Adam


On Thu, Aug 30, 2012 at 5:25 PM, Jeremy Apthorp <jeremya@chromium.org> wrote:
> platform_app.js should be run at a fairly normal time (along with all the
> other extension JS that gets run when a page is a created).
> inject_app_titlebar.js is the weird one, and it looks like the
> onreadystatechange event with capture fires at the right time. So I should
> be able to remove the DidCreateDocumentElement listener from the C++ :)
>
>
> On Fri, Aug 31, 2012 at 10:23 AM, Adam Barth <abarth@chromium.org> wrote:
>>
>> On Thu, Aug 30, 2012 at 4:05 PM, Mihai Parparita <mihaip@chromium.org>
>> wrote:
>> > On Wed, Aug 29, 2012 at 6:50 PM, <jeremya@chromium.org> wrote:
>> >>
>> >> (and also: readystatechange doesn't bubble to window, so I can't add a
>> >> listener on the window. The problem here is that the document element
>> >> gets substituted out when the navigation happens, so any event
>> >> listeners
>> >> I register on the document at window creation time are gone by the time
>> >> the second document gets loaded.)
>> >
>> >
>> > You don't actually need to to bubble to the window, you can listen for
>> > it at
>> > the window level in the capturing phase. Something like
>> >
>> > window.addEventListener('readystatechange', function() { ...}, true /*
>> > true
>> > == capture */);
>> >
>> > I experimented with that in in platform_app.js, and as far as I can
>> > tell,
>> > while platform_app.js is being run, document points to the old document
>> > (as
>> > you said). However, if I add an event listener as above to the window,
>> > it
>> > will fire (with the document in event.target) with the new document
>> > before
>> > the page has had any chance to run its code, thus it seems like a good
>> > time
>> > to do the titlebar injection.
>>
>> I'm concerned that we're running platform_app.js at a super strange
>> time in the middle of navigation.  I mentioned this before, but I'm
>> worried that the more complexity we pile into platform_app.js the more
>> fragile it will become and the harder it will be to fix it later.
>>
>> Can we provide you better APIs to call you back at exactly the right
>> time so you don't need to resort to this sort of hackery?
>>
>> Adam
>
>

jeremya

Changed around the JS a little and refactored the method call stuff into module_system.cc. Please ...

8 years, 3 months ago (2012-08-31 03:50:44 UTC) #17

jeremya

On 2012/08/31 03:50:44, jeremya wrote: > Changed around the JS a little and refactored the ...

8 years, 3 months ago (2012-08-31 06:34:02 UTC) #18

Mihai Parparita -not on Chrome

https://chromiumcodereview.appspot.com/10896032/diff/11003/chrome/browser/extensions/api/app_window/app_window_api.cc File chrome/browser/extensions/api/app_window/app_window_api.cc (left): https://chromiumcodereview.appspot.com/10896032/diff/11003/chrome/browser/extensions/api/app_window/app_window_api.cc#oldcode73 chrome/browser/extensions/api/app_window/app_window_api.cc:73: ShellWindow::CreateParams::FRAME_CHROME; Can you file a cleanup bug to remove ...

8 years, 3 months ago (2012-08-31 23:12:56 UTC) #19

jeremya

8 years, 3 months ago (2012-09-01 01:42:30 UTC) #20

Mihai Parparita -not on Chrome

LGTM Can you include a brief description of how HTML titlebars are implemented in the ...

8 years, 3 months ago (2012-09-04 04:01:36 UTC) #22

jeremya

Done and done. I think I might have to land this patch directly to circumvent ...

8 years, 3 months ago (2012-09-04 09:42:54 UTC) #23

Mihai Parparita -not on Chrome

It occurred to me that you'll need to update ChromeRenderViewObserver::allowWebComponents to always return true for ...

8 years, 3 months ago (2012-09-04 22:49:40 UTC) #24

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/10896032/18007

8 years, 3 months ago (2012-09-05 01:14:39 UTC) #26

commit-bot: I haz the power

Presubmit check for 10896032-18007 failed and returned exit status 1. Running presubmit commit checks ...

8 years, 3 months ago (2012-09-05 01:14:46 UTC) #27

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/10896032/18008

8 years, 3 months ago (2012-09-05 01:26:14 UTC) #28

commit-bot: I haz the power

Try job failure for 10896032-18008 (retry) on mac_rel for step "browser_tests". It's a second try, ...

8 years, 3 months ago (2012-09-05 03:24:49 UTC) #29

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/10896032/16004

8 years, 3 months ago (2012-09-05 04:37:18 UTC) #30

lgtm

Expand Messages | Collapse Messages