Issue 11238055: Re-open DevTools windows for PackagedApps if they are reloaded with DevTools open.

Issue 11238055: Re-open DevTools windows for PackagedApps if they are reloaded with DevTools open. (Closed)

Created:
8 years, 2 months ago by tapted

Modified:
8 years, 2 months ago

Reviewers:
benwells

CC:
chromium-reviews, Aaron Boodman, chromium-apps-reviews_chromium.org, koz (OOO until 15th September), jeremya

Base URL:
http://git.chromium.org/chromium/src.git@master

Visibility:
Public.

More Reviews

Description

Re-open DevTools windows for PackagedApps if they are reloaded with DevTools open. This augments the ShellWindowRegistry with a set of <extension_id, window_id> keys that were observed to have devtools attached to them, and delays returning to the chrome.app.window.create(..) callback until a new devtools window is open and ready to intercept any breakpoints that were specified in the client code. BUG=153235 TEST=Run packaged App that uses |id| in create(..), Inspect, set breakpoint, reload app. Result: inspector reloads with app. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=163779

Patch Set 1 : before unit test #

Total comments: 21

Patch Set 2 : reviewer comments #

Patch Set 3 : picky visual stdio #

Patch Set 4 : adds unit test but hits http://crbug.com/157097 on debug builds #

Total comments: 3

Patch Set 5 : more encapsulation, self-nit unit test code, add expected FAILS_ for debug builds #

Total comments: 13

Patch Set 6 : reviewer comments, use MAYBE_ #

Created: 8 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+211 lines, -12 lines)			Patch
M	chrome/browser/extensions/api/app_window/app_window_api.h	View	1 2 3 4	1 chunk	+3 lines, -1 line	0 comments	Download
M	chrome/browser/extensions/api/app_window/app_window_api.cc	View	1 2 3 4 5	3 chunks	+57 lines, -1 line	0 comments	Download
M	chrome/browser/extensions/platform_app_browsertest.cc	View	1 2 3 4 5	3 chunks	+57 lines, -1 line	0 comments	Download
M	chrome/browser/extensions/shell_window_registry.h	View		5 chunks	+19 lines, -2 lines	0 comments	Download
M	chrome/browser/extensions/shell_window_registry.cc	View	1 2	3 chunks	+61 lines, -2 lines	0 comments	Download
A	chrome/test/data/extensions/platform_apps/minimal_id/main.html	View	1 2 3 4	1 chunk	+10 lines, -0 lines	0 comments	Download
A +	chrome/test/data/extensions/platform_apps/minimal_id/main.js	View	1 2 3 4	1 chunk	+2 lines, -0 lines	0 comments	Download
A +	chrome/test/data/extensions/platform_apps/minimal_id/manifest.json	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download
A +	chrome/test/data/extensions/platform_apps/minimal_id/test.js	View	1 2 3 4 5	1 chunk	+1 line, -4 lines	0 comments	Download

Messages

Total messages: 10 (0 generated)

Expand Messages | Collapse Messages

tapted

still investigating a possible/feasible unit test https://codereview.chromium.org/11238055/diff/2001/chrome/browser/extensions/shell_window_registry.h File chrome/browser/extensions/shell_window_registry.h (right): https://codereview.chromium.org/11238055/diff/2001/chrome/browser/extensions/shell_window_registry.h#newcode51 chrome/browser/extensions/shell_window_registry.h:51: typedef std::set<std::string> InspectedWindowSet; ...

8 years, 2 months ago (2012-10-23 06:04:14 UTC) #1

benwells

https://codereview.chromium.org/11238055/diff/2001/chrome/browser/extensions/api/app_window/app_window_api.cc File chrome/browser/extensions/api/app_window/app_window_api.cc (right): https://codereview.chromium.org/11238055/diff/2001/chrome/browser/extensions/api/app_window/app_window_api.cc#newcode42 chrome/browser/extensions/api/app_window/app_window_api.cc:42: class AppWindowCreateFunction::DelayedResponseForDevTools Hmmm, naming is always hard, but I ...

8 years, 2 months ago (2012-10-23 06:18:48 UTC) #2

tapted

Amalgamate review. Might try adapting my unit test from https://codereview.chromium.org/11190016 ... https://codereview.chromium.org/11238055/diff/2001/chrome/browser/extensions/api/app_window/app_window_api.cc File chrome/browser/extensions/api/app_window/app_window_api.cc (right): ...

8 years, 2 months ago (2012-10-23 07:04:23 UTC) #3

Amalgamate review. Might try adapting my unit test from
https://codereview.chromium.org/11190016 ...

https://codereview.chromium.org/11238055/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/11238055/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/app_window/app_window_api.cc:42: class
AppWindowCreateFunction::DelayedResponseForDevTools
On 2012/10/23 06:18:48, benwells wrote:
> Hmmm, naming is always hard, but I think we can do better here. Perhaps
> DevToolsRestorer? Nouns are good but this thing isn't the response.

Done. (I actually picked the name in an early version that had this class
holding the base::DictionaryResult* result).

https://codereview.chromium.org/11238055/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/app_window/app_window_api.cc:47: parent_(parent) {
On 2012/10/23 06:18:48, benwells wrote:
> nit: colon goes on this line, like so:
>     : parent_(parent) {

Done.

https://codereview.chromium.org/11238055/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/app_window/app_window_api.cc:48: 
On 2012/10/23 06:18:48, benwells wrote:
> nit: remove blank line.

Done.

https://codereview.chromium.org/11238055/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/app_window/app_window_api.cc:202:
delay_response_.reset(new DelayedResponseForDevTools(this, created_view));
On 2012/10/23 06:18:48, benwells wrote:
> nit: early return true here instead of else. 

Done.

https://codereview.chromium.org/11238055/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/shell_window_registry.cc (right):

https://codereview.chromium.org/11238055/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/shell_window_registry.cc:13: namespace {
On 2012/10/23 06:18:48, benwells wrote:
> Nit: blank line after namespace

Done.

https://codereview.chromium.org/11238055/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/shell_window_registry.cc:20: return std::string(); //
Not a ShellWindow.
On 2012/10/23 06:18:48, benwells wrote:
> Blank line after early return.

Done.

https://codereview.chromium.org/11238055/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/shell_window_registry.cc:24: std::string key =
shell_window->extension()->id();
On 2012/10/23 06:18:48, benwells wrote:
> Please put a comment in describing the key format. It might be neater to put a
> colon in between the two fields.

Done.

https://codereview.chromium.org/11238055/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/shell_window_registry.cc:27: }
On 2012/10/23 06:18:48, benwells wrote:
> Nit: blank line between }'s

Done.

https://codereview.chromium.org/11238055/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/shell_window_registry.cc:126: if (type ==
content::NOTIFICATION_DEVTOOLS_AGENT_ATTACHED) {
On 2012/10/23 06:18:48, benwells wrote:
> Use switch here and put NOT_REACHED for default case.

Done.

https://codereview.chromium.org/11238055/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/shell_window_registry.h (right):

https://codereview.chromium.org/11238055/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/shell_window_registry.h:82: bool
HadDevToolsAttached(content::RenderViewHost* render_view_host) const;
On 2012/10/23 06:18:48, benwells wrote:
> Where is the logic that tracks window positions to restore? Maybe this could
go
> there too?

It's in extensions::ShellWindowGeometryCache and accessed via

ShellWindowGeometryCache* ExtensionSystem::shell_window_geometry_cache()

One difference is that this cache is a thing persisted to disk, and read back
from disk at chrome launch, whereas ShellWindowRegistry is an ephemeral thing. I
think devtools "open" status should be ephemeral too. 

I only have a weak argument against it being a new, ephemeral part of
ShellWindowGeometryCache. An implementation in ShellWindowGeometryCache would
still have to use ShellWindowRegistry, since the "loaded" observer on the
DevToolsWindow only knows about the RenderViewHost that it's inspecting, and we
need ShellWindowRegistry to turn that into an extension+ShellWindow (and
ShellWindowGeometryCache doesn't use the registry currently). Also
ShellWindowRegistry's class comment seemed to fit ;-)

tapted

Unit test for the try-bots, and general feedback, but let me self-nit the additions first, ...

8 years, 2 months ago (2012-10-23 10:16:45 UTC) #4

Unit test for the try-bots, and general feedback, but let me self-nit the
additions first, (if there is some way to keep them without breaking tests due
to the segfault in webkit..).

https://codereview.chromium.org/11238055/diff/5006/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

https://codereview.chromium.org/11238055/diff/5006/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_browsertest.cc:732:
CloseShellWindow(window);
This line triggers http://crbug.com/157097 when (WebKit is) compiled with NDEBUG
(release builds don't have the funky pointer dereference).

Should we add

#ifdef NDEBUG
EXPECT_FAIL (..)
#else
EXPECT_PASS (..)
#endif

?


Expecting (any non-release?) trybots to spew out some of these:

[32110:32110:1023/205245:ERROR:process_util_posix.cc(144)] Received signal 11
	base::debug::StackTrace::StackTrace() [0x7f62da6008e6]
	base::(anonymous namespace)::StackDumpSignalHandler() [0x7f62da664829]
	0x7f62cddeb4a0
	WebCore::GraphicsLayer::willBeDestroyed() [0x7f62d68ce806]
	WebCore::GraphicsLayerChromium::willBeDestroyed() [0x7f62d68f25ca]
	WebCore::GraphicsLayerChromium::~GraphicsLayerChromium() [0x7f62d68f2480]
	WebCore::GraphicsLayerChromium::~GraphicsLayerChromium() [0x7f62d68f2560]
	WTF::deleteOwnedPtr<>() [0x7f62d5d2c2d4]
	WTF::OwnPtr<>::~OwnPtr() [0x7f62d5d2c145]
	WebKit::PageOverlay::~PageOverlay() [0x7f62d5d342bc]
	WTF::deleteOwnedPtr<>() [0x7f62d5d347a0]
	WTF::OwnPtr<>::~OwnPtr() [0x7f62d5d34417]
	WTF::VectorDestructor<>::destruct() [0x7f62d5d34bd2]
	WTF::VectorTypeOperations<>::destruct() [0x7f62d5d34ac2]
	WTF::Vector<>::shrink() [0x7f62d5d34770]
	WTF::Vector<>::~Vector() [0x7f62d5d343a5]
	WebKit::PageOverlayList::~PageOverlayList() [0x7f62d5d33d92]
	WTF::deleteOwnedPtr<>() [0x7f62d5de0602]
	WTF::OwnPtr<>::~OwnPtr() [0x7f62d5dde421]
	WebKit::WebViewImpl::~WebViewImpl() [0x7f62d5dcd4c6]
	WebKit::WebViewImpl::~WebViewImpl() [0x7f62d5dcd676]
	WTF::RefCounted<>::deref() [0x7f62d5ddf058]
	WebKit::WebViewImpl::close() [0x7f62d5dd1a8c]
	content::RenderWidget::Close() [0x7f62d36f163c]
	content::RenderViewImpl::Close() [0x7f62d36d241a]

benwells

https://codereview.chromium.org/11238055/diff/5006/chrome/browser/extensions/platform_app_browsertest.cc File chrome/browser/extensions/platform_app_browsertest.cc (right): https://codereview.chromium.org/11238055/diff/5006/chrome/browser/extensions/platform_app_browsertest.cc#newcode732 chrome/browser/extensions/platform_app_browsertest.cc:732: CloseShellWindow(window); On 2012/10/23 10:16:45, tapted wrote: > This line ...

8 years, 2 months ago (2012-10-23 10:27:13 UTC) #5

https://codereview.chromium.org/11238055/diff/5006/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

https://codereview.chromium.org/11238055/diff/5006/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_browsertest.cc:732:
CloseShellWindow(window);
On 2012/10/23 10:16:45, tapted wrote:
> This line triggers http://crbug.com/157097 when (WebKit is) compiled with
NDEBUG
> (release builds don't have the funky pointer dereference).
> 
> Should we add
> 
> #ifdef NDEBUG
> EXPECT_FAIL (..)
> #else
> EXPECT_PASS (..)
> #endif
> 
> ?
> 
> 
> Expecting (any non-release?) trybots to spew out some of these:
> 
> [32110:32110:1023/205245:ERROR:process_util_posix.cc(144)] Received signal 11
> 	base::debug::StackTrace::StackTrace() [0x7f62da6008e6]
> 	base::(anonymous namespace)::StackDumpSignalHandler() [0x7f62da664829]
> 	0x7f62cddeb4a0
> 	WebCore::GraphicsLayer::willBeDestroyed() [0x7f62d68ce806]
> 	WebCore::GraphicsLayerChromium::willBeDestroyed() [0x7f62d68f25ca]
> 	WebCore::GraphicsLayerChromium::~GraphicsLayerChromium() [0x7f62d68f2480]
> 	WebCore::GraphicsLayerChromium::~GraphicsLayerChromium() [0x7f62d68f2560]
> 	WTF::deleteOwnedPtr<>() [0x7f62d5d2c2d4]
> 	WTF::OwnPtr<>::~OwnPtr() [0x7f62d5d2c145]
> 	WebKit::PageOverlay::~PageOverlay() [0x7f62d5d342bc]
> 	WTF::deleteOwnedPtr<>() [0x7f62d5d347a0]
> 	WTF::OwnPtr<>::~OwnPtr() [0x7f62d5d34417]
> 	WTF::VectorDestructor<>::destruct() [0x7f62d5d34bd2]
> 	WTF::VectorTypeOperations<>::destruct() [0x7f62d5d34ac2]
> 	WTF::Vector<>::shrink() [0x7f62d5d34770]
> 	WTF::Vector<>::~Vector() [0x7f62d5d343a5]
> 	WebKit::PageOverlayList::~PageOverlayList() [0x7f62d5d33d92]
> 	WTF::deleteOwnedPtr<>() [0x7f62d5de0602]
> 	WTF::OwnPtr<>::~OwnPtr() [0x7f62d5dde421]
> 	WebKit::WebViewImpl::~WebViewImpl() [0x7f62d5dcd4c6]
> 	WebKit::WebViewImpl::~WebViewImpl() [0x7f62d5dcd676]
> 	WTF::RefCounted<>::deref() [0x7f62d5ddf058]
> 	WebKit::WebViewImpl::close() [0x7f62d5dd1a8c]
> 	content::RenderWidget::Close() [0x7f62d36f163c]
> 	content::RenderViewImpl::Close() [0x7f62d36d241a]

#if NDEBUG sg, with a comment linking to the bug.

tapted

I think this is ready for final review. https://codereview.chromium.org/11238055/diff/5006/chrome/browser/extensions/platform_app_browsertest.cc File chrome/browser/extensions/platform_app_browsertest.cc (right): https://codereview.chromium.org/11238055/diff/5006/chrome/browser/extensions/platform_app_browsertest.cc#newcode732 chrome/browser/extensions/platform_app_browsertest.cc:732: CloseShellWindow(window); ...

8 years, 2 months ago (2012-10-23 23:41:15 UTC) #6

I think this is ready for final review.

https://codereview.chromium.org/11238055/diff/5006/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

https://codereview.chromium.org/11238055/diff/5006/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_browsertest.cc:732:
CloseShellWindow(window);
On 2012/10/23 10:27:14, benwells wrote:
> On 2012/10/23 10:16:45, tapted wrote:
> > This line triggers http://crbug.com/157097 when (WebKit is) compiled with
> NDEBUG
> > (release builds don't have the funky pointer dereference).
> > 
> > Should we add
> > 
> > #ifdef NDEBUG
> > EXPECT_FAIL (..)
> > #else
> > EXPECT_PASS (..)
> > #endif
> > 
> > ?
> > 
> > 
> > Expecting (any non-release?) trybots to spew out some of these:
> > 
> > [32110:32110:1023/205245:ERROR:process_util_posix.cc(144)] Received signal
11
> > 	base::debug::StackTrace::StackTrace() [0x7f62da6008e6]
> > 	base::(anonymous namespace)::StackDumpSignalHandler() [0x7f62da664829]
> > 	0x7f62cddeb4a0
> > 	WebCore::GraphicsLayer::willBeDestroyed() [0x7f62d68ce806]
> > 	WebCore::GraphicsLayerChromium::willBeDestroyed() [0x7f62d68f25ca]
> > 	WebCore::GraphicsLayerChromium::~GraphicsLayerChromium() [0x7f62d68f2480]
> > 	WebCore::GraphicsLayerChromium::~GraphicsLayerChromium() [0x7f62d68f2560]
> > 	WTF::deleteOwnedPtr<>() [0x7f62d5d2c2d4]
> > 	WTF::OwnPtr<>::~OwnPtr() [0x7f62d5d2c145]
> > 	WebKit::PageOverlay::~PageOverlay() [0x7f62d5d342bc]
> > 	WTF::deleteOwnedPtr<>() [0x7f62d5d347a0]
> > 	WTF::OwnPtr<>::~OwnPtr() [0x7f62d5d34417]
> > 	WTF::VectorDestructor<>::destruct() [0x7f62d5d34bd2]
> > 	WTF::VectorTypeOperations<>::destruct() [0x7f62d5d34ac2]
> > 	WTF::Vector<>::shrink() [0x7f62d5d34770]
> > 	WTF::Vector<>::~Vector() [0x7f62d5d343a5]
> > 	WebKit::PageOverlayList::~PageOverlayList() [0x7f62d5d33d92]
> > 	WTF::deleteOwnedPtr<>() [0x7f62d5de0602]
> > 	WTF::OwnPtr<>::~OwnPtr() [0x7f62d5dde421]
> > 	WebKit::WebViewImpl::~WebViewImpl() [0x7f62d5dcd4c6]
> > 	WebKit::WebViewImpl::~WebViewImpl() [0x7f62d5dcd676]
> > 	WTF::RefCounted<>::deref() [0x7f62d5ddf058]
> > 	WebKit::WebViewImpl::close() [0x7f62d5dd1a8c]
> > 	content::RenderWidget::Close() [0x7f62d36f163c]
> > 	content::RenderViewImpl::Close() [0x7f62d36d241a]
> 
> #if NDEBUG sg, with a comment linking to the bug.

Done.

https://codereview.chromium.org/11238055/diff/8003/chrome/browser/extensions/...
File chrome/browser/extensions/api/app_window/app_window_api.h (right):

https://codereview.chromium.org/11238055/diff/8003/chrome/browser/extensions/...
chrome/browser/extensions/api/app_window/app_window_api.h:16: void
SendDelayedResponse();
tossed this up for a while.. `SendResponse` is protected, so if we want to move
the observer into an anonymous namespace to reduce symbol bloat, it needs to be
exposed here.

benwells

lgtm with nits and assuming tryjobs are happy http://codereview.chromium.org/11238055/diff/8003/chrome/browser/extensions/api/app_window/app_window_api.cc File chrome/browser/extensions/api/app_window/app_window_api.cc (right): http://codereview.chromium.org/11238055/diff/8003/chrome/browser/extensions/api/app_window/app_window_api.cc#newcode50 chrome/browser/extensions/api/app_window/app_window_api.cc:50: DevToolsWindow::ToggleDevToolsWindow(created_view, ...

8 years, 2 months ago (2012-10-24 03:24:08 UTC) #7

tapted

scraping logs, stdio on {linux,mac,win}{,rel} suggest that PlatformAppBrowserTest.{FAILS_,}DevToolsOpenedWithReload is content with this approach, even though ...

8 years, 2 months ago (2012-10-24 04:03:57 UTC) #8

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tapted@chromium.org/11238055/19002

8 years, 2 months ago (2012-10-24 04:06:02 UTC) #9

Change committed as 163779

Expand Messages | Collapse Messages