Issue 331563003: Launching + Views

Issue 331563003: Launching + Views (Closed)

Created:
6 years, 6 months ago by Ben Goodger (Google)

Modified:
6 years, 6 months ago

Reviewers:
Aaron Boodman

CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, darin (slow to review), ben+mojo_chromium.org

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

Visibility:
Public.

More Reviews

Description

Adds a parameter to Launchable that is a ViewTreeNode to render in. Adds an image viewer app that decodes the provided data stream as PNG and renders it in a View associated with the supplied node. R=aa@chromium.org, sky@chromium.org http://crbug.com/378151 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277084

Patch Set 9 : . #

Created: 6 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+372 lines, -39 lines)			Patch
M	mojo/examples/browser/browser.cc	View	1 2 3 4 5 6 7 8	5 chunks	+80 lines, -9 lines	0 comments	Download
M	mojo/examples/html_viewer/html_viewer.cc	View	1 2 3	3 chunks	+40 lines, -6 lines	0 comments	Download
A +	mojo/examples/image_viewer/DEPS	View	1 2 3	1 chunk	+0 lines, -2 lines	0 comments	Download
A	mojo/examples/image_viewer/image_viewer.cc	View	1 2 3 4 5 6 7	1 chunk	+137 lines, -0 lines	0 comments	Download
M	mojo/examples/window_manager/window_manager.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	mojo/mojo.gyp	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	mojo/mojo_examples.gypi	View	1 2 3	1 chunk	+25 lines, -3 lines	0 comments	Download
M	mojo/services/launcher/launcher.cc	View	1 2 3 4 5	6 chunks	+18 lines, -16 lines	0 comments	Download
M	mojo/services/public/cpp/input_events/lib/input_events_type_converters.cc	View	1	1 chunk	+2 lines, -1 line	0 comments	Download
M	mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.h	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc	View	1 2 3	2 chunks	+27 lines, -0 lines	0 comments	Download
M	mojo/services/public/cpp/view_manager/lib/view_tree_node.cc	View	1 2 3	1 chunk	+5 lines, -0 lines	0 comments	Download
M	mojo/services/public/cpp/view_manager/view_tree_node.h	View	1 2 3	1 chunk	+3 lines, -0 lines	0 comments	Download
M	mojo/services/public/interfaces/launcher/launcher.mojom	View		1 chunk	+10 lines, -1 line	0 comments	Download
M	mojo/services/public/interfaces/view_manager/view_manager.mojom	View	1 2 3	1 chunk	+3 lines, -0 lines	0 comments	Download
M	mojo/services/view_manager/view_manager_connection.h	View	1 2 3	2 chunks	+3 lines, -0 lines	0 comments	Download
M	mojo/services/view_manager/view_manager_connection.cc	View	1 2 3	2 chunks	+16 lines, -0 lines	0 comments	Download

Messages

Total messages: 9 (0 generated)

Expand Messages | Collapse Messages

Ben Goodger (Google)

On 2014/06/13 18:16:54, Ben Goodger (Google) wrote: actually i noticed a problem. one sec.

6 years, 6 months ago (2014-06-13 19:31:15 UTC) #2

Ben Goodger (Google)

On 2014/06/13 19:31:15, Ben Goodger (Google) wrote: > On 2014/06/13 18:16:54, Ben Goodger (Google) wrote: ...

6 years, 6 months ago (2014-06-13 19:45:15 UTC) #3

Aaron Boodman

lgtm https://codereview.chromium.org/331563003/diff/140001/mojo/examples/browser/browser.cc File mojo/examples/browser/browser.cc (right): https://codereview.chromium.org/331563003/diff/140001/mojo/examples/browser/browser.cc#newcode59 mojo/examples/browser/browser.cc:59: views::View* text_field = host->child_at(0); weird to fish views ...

6 years, 6 months ago (2014-06-13 20:09:52 UTC) #5

Ben Goodger (Google)

Thanks! https://codereview.chromium.org/331563003/diff/140001/mojo/examples/browser/browser.cc File mojo/examples/browser/browser.cc (right): https://codereview.chromium.org/331563003/diff/140001/mojo/examples/browser/browser.cc#newcode59 mojo/examples/browser/browser.cc:59: views::View* text_field = host->child_at(0); Yeah that's what you'd ...

6 years, 6 months ago (2014-06-13 20:29:28 UTC) #6

Thanks!

https://codereview.chromium.org/331563003/diff/140001/mojo/examples/browser/b...
File mojo/examples/browser/browser.cc (right):

https://codereview.chromium.org/331563003/diff/140001/mojo/examples/browser/b...
mojo/examples/browser/browser.cc:59: views::View* text_field =
host->child_at(0);
Yeah that's what you'd typically do, but this is shitty code ;-)

I figured whoever takes ownership of this browser app would do a nice job
figuring out what the code should look like. I think there'll be more views in
the fullness of time.

https://codereview.chromium.org/331563003/diff/140001/mojo/examples/browser/b...
mojo/examples/browser/browser.cc:163: view_manager::ViewTreeNode* content_node_;
On 2014/06/13 20:09:52, Aaron Boodman wrote:
> content_node_ needs an initializer.

Done.

https://codereview.chromium.org/331563003/diff/140001/mojo/examples/image_vie...
File mojo/examples/image_viewer/image_viewer.cc (right):

https://codereview.chromium.org/331563003/diff/140001/mojo/examples/image_vie...
mojo/examples/image_viewer/image_viewer.cc:58: content_length, &bitmap);
On 2014/06/13 20:09:52, Aaron Boodman wrote:
> Wow, this is a cool outcome of Mojo's design.
> 
> When I first saw this my gut reaction was "yikes! security blah blah". But
then
> I realized actually this whole process would be sandboxed, so it's not a
> problem. Neat.

Yeah it's interesting. We do this in chrome today for a few things.. e.g.
profile import is done in a sandboxed process.

https://codereview.chromium.org/331563003/diff/140001/mojo/examples/image_vie...
mojo/examples/image_viewer/image_viewer.cc:68: base::StringTokenizer
t(headers[i], ": ;=");
On 2014/06/13 20:09:52, Aaron Boodman wrote:
> Probably right thing to do here is either reuse Blink entirely, or at least
> header parsing code from net.

Agreed, esp as this is the second time I've copy-pasted this in my demo apps.
Question is whether or not we want this code to depend on net via http_util...
or if we want to build out the utils in mojo/services/public/cpp/network. Filed
http://crbug.com/384530

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ben@chromium.org/331563003/160001

6 years, 6 months ago (2014-06-13 21:24:02 UTC) #8

Ben Goodger (Google)

6 years, 6 months ago (2014-06-13 21:39:13 UTC) #9

Message was sent while issue was closed.

Committed patchset #9 manually as r277084 (presubmit successful).

Expand Messages | Collapse Messages