Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1295243003:
Initial commit of the blimp/ folder and target (Closed)
|
Created:
5 years, 4 months ago by David Trainor- moved to gerrit Modified:
5 years, 3 months ago Reviewers:
Dirk Pranke, Kevin Marshall, Wez, jbudorick, reed2, enne (OOO), danakj, nyquist, no sievers, Khushal, reed1 CC:
chromium-reviews, Khushal Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInitial commit of the blimp/ folder and target
- Sets up a basic Android compositor shell.
- Eventually should support other platforms as well.
Inspiration for most components:
BlimpLibraryLoader:
BrowserStartupController.java
blimp_library_loader.[cc,h]:
content/app/android/library_loader_hooks.[cc,h]
content/app/content_main_runner.[h,cc]
blimp_jni_registrar.[cc,h]:
content/app/android/app_jni_registrar.[cc,h]
BlimpView.java:
CompositorView.java
blimp_view.[cc,h]:
compositor_view.[cc,h]
blimp_compositor.[cc,h]/blimp_output_surface.[cc,h]
compositor_impl_android.[cc,h]
blimp_context_provider.[cc,h]:
in_process_context_provider.[cc,h]
aw_render_thread_context_provider.[cc,h]
blimp_layer_tree_settings.[cc,h]:
render_widget_compositor.[cc,h]
BUG=526219
Committed: https://crrev.com/e9c78dddbb858895a13ed8b22c53a07d63a9ff1b
Cr-Commit-Position: refs/heads/master@{#347223}
Patch Set 1 #
Total comments: 47
Patch Set 2 : Cleaned it up, addressed nyquist@ nits #
Total comments: 9
Patch Set 3 : #Patch Set 4 : Move target to right place in all.gyp #Patch Set 5 : Removed unnecessary deps entry for this patch #Patch Set 6 : Some cleanup nits. #
Total comments: 145
Patch Set 7 : Added GN targets and addressed some nits #Patch Set 8 : #
Total comments: 34
Patch Set 9 : Addressed more nits/comments #Patch Set 10 : Moar nitz #
Total comments: 8
Patch Set 11 : Fix deps, fix check #Patch Set 12 : Rebased #Patch Set 13 : Remove unused LayerTreeHostSingleThreadClient #Patch Set 14 : Removed DCHECK, removed duplicate include. #
Total comments: 6
Patch Set 15 : Removed unused include #Patch Set 16 : rebased #
Total comments: 61
Patch Set 17 : Address more nits #
Total comments: 21
Patch Set 18 : Address final nits. #
Total comments: 2
Messages
Total messages: 63 (9 generated)
dtrainor@chromium.org changed reviewers: + marshallk@google.com, nyquist@chromium.org, wez@chromium.org
Initial patch that builds a small compositor shell. I need to clean this up a bunch, but this does seem to build and paint to the screen. I'll work on pulling some of the compositor bits outside of the android folder since we'll need that for a linux shell if we want that too. We should find a good way of abstracting the window (can we use what the rest of Chrome uses?) so that this will work for most platforms... although I *really* want to avoid including any code we don't have to - at least for the Android version.
https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/blimp.gyp File blimp/blimp.gyp (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/blimp.gyp#newc... blimp/blimp.gyp:18: '../cc/cc.gyp:cc', Nit: lexicographic sort https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/DEPS File blimp/client/DEPS (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/DEPS#ne... blimp/client/DEPS:4: "+content/public/common", Could we have "-content", "!content/public/common/content_switches.h" instead? I think I would find it helpful if we instead clearly disallowed content with -content and then the temporary rule for the switches until we remove that dependency. Or do you think we will end up needing a lot of //content? https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... File blimp/client/android/blimp_library_loader.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/blimp_library_loader.h:24: #include <jni.h> Nit: This might not be enough. Add a couple more maybe? https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... File blimp/client/android/blimp_view.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/blimp_view.h:20: class BlimpView { Nit: Maybe a little comment here to clarify what this view is about? Or possibly just refer to the Java-class? https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/blimp_view.h:43: protected: Nit: This seems unnecessary? https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... File blimp/client/android/compositor/blimp_compositor.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/compositor/blimp_compositor.cc:137: // We don't know the exact resolution due to screen controls etc. Optional nit: Add comma at end of line, and lowercase 'so'? https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/compositor/blimp_compositor.cc:335: if (has_synchronous_compositor_factory) { Would we be certain to get one of these blocks always going forward? https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... File blimp/client/android/compositor/blimp_compositor.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/compositor/blimp_compositor.h:29: class BlimpCompositor : public cc::LayerTreeHostClient, Nit: A little class comment might be helpful here. What are the responsibilities? https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/compositor/blimp_compositor.h:38: void SetSurface(JNIEnv* env, jobject jsurface); Like you mentioned yourself, it'd be great to have this class as the cross-platform parts of the code, so maybe extract things? https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... File blimp/client/android/compositor/blimp_context_provider.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/compositor/blimp_context_provider.h:47: BlimpContextProvider(gfx::AcceleratedWidget widget); Nit: Single-argument constructor == explicit? https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... File blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java:21: public class BlimpLibraryLoader { Nit: Since this class has a private constructor, and only static methods, do we want to make the class final? https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java:42: * @param callback A {@link BlimpLibraryLoader#Callback} to be notified upon Nit: Callback is an interface, not method, so I think it should be {@link BlimpLibraryLoader.Callback} https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java:60: boolean startResult = nativeStartBlimp(); Do we need to try to start blimp, if init failed? https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... File blimp/client/android/java/src/org/chromium/blimp/BlimpRendererActivity.java (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:25: // LibraryLoader.get(LibraryProcessType.PROCESS_BROWSER).ensureInitialized(this); Nit: I guess this is supposed to be removed? https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:28: Log.e(TAG, "Native startup exception"); Nit: Would we want to include the exception here? I don't think it should normally cause an exception, so it should only be logged if there in fact is an issue. https://chromiumcodereview.appspot.com/1295243003/diff/1/ui/gfx/gfx.gyp File ui/gfx/gfx.gyp (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/ui/gfx/gfx.gyp#newco... ui/gfx/gfx.gyp:105: # 'test/gfx_util.h', Why do we need to include a test file?
https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... File blimp/client/android/blimp_library_loader.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/blimp_library_loader.h:24: #include <jni.h> On 2015/08/19 07:48:06, nyquist wrote: > Nit: This might not be enough. Add a couple more maybe? WTF vim ftl :(.
https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/blimp.gyp File blimp/blimp.gyp (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/blimp.gyp#newc... blimp/blimp.gyp:18: '../cc/cc.gyp:cc', On 2015/08/19 07:48:06, nyquist (OOO - back 8-24) wrote: > Nit: lexicographic sort Done. https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/DEPS File blimp/client/DEPS (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/DEPS#ne... blimp/client/DEPS:4: "+content/public/common", On 2015/08/19 07:48:06, nyquist (OOO - back 8-24) wrote: > Could we have > "-content", > "!content/public/common/content_switches.h" instead? I think I would find it > helpful if we instead clearly disallowed content with -content and then the > temporary rule for the switches until we remove that dependency. > > Or do you think we will end up needing a lot of //content? Good idea! https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... File blimp/client/android/blimp_view.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/blimp_view.h:20: class BlimpView { On 2015/08/19 07:48:06, nyquist (OOO - back 8-24) wrote: > Nit: Maybe a little comment here to clarify what this view is about? Or possibly > just refer to the Java-class? Done. https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/blimp_view.h:43: protected: On 2015/08/19 07:48:06, nyquist (OOO - back 8-24) wrote: > Nit: This seems unnecessary? Done. https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... File blimp/client/android/compositor/blimp_compositor.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/compositor/blimp_compositor.cc:137: // We don't know the exact resolution due to screen controls etc. On 2015/08/19 07:48:06, nyquist (OOO - back 8-24) wrote: > Optional nit: Add comma at end of line, and lowercase 'so'? Done. https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... File blimp/client/android/compositor/blimp_compositor.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/compositor/blimp_compositor.h:29: class BlimpCompositor : public cc::LayerTreeHostClient, On 2015/08/19 07:48:06, nyquist (OOO - back 8-24) wrote: > Nit: A little class comment might be helpful here. What are the > responsibilities? Done. https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/compositor/blimp_compositor.h:38: void SetSurface(JNIEnv* env, jobject jsurface); On 2015/08/19 07:48:06, nyquist (OOO - back 8-24) wrote: > Like you mentioned yourself, it'd be great to have this class as the > cross-platform parts of the code, so maybe extract things? Done. https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... File blimp/client/android/compositor/blimp_context_provider.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/compositor/blimp_context_provider.h:47: BlimpContextProvider(gfx::AcceleratedWidget widget); On 2015/08/19 07:48:06, nyquist (OOO - back 8-24) wrote: > Nit: Single-argument constructor == explicit? Done. https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... File blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java:21: public class BlimpLibraryLoader { On 2015/08/19 07:48:07, nyquist (OOO - back 8-24) wrote: > Nit: Since this class has a private constructor, and only static methods, do we > want to make the class final? Done. https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java:42: * @param callback A {@link BlimpLibraryLoader#Callback} to be notified upon On 2015/08/19 07:48:07, nyquist (OOO - back 8-24) wrote: > Nit: Callback is an interface, not method, so I think it should be {@link > BlimpLibraryLoader.Callback} Done. https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java:60: boolean startResult = nativeStartBlimp(); On 2015/08/19 07:48:07, nyquist (OOO - back 8-24) wrote: > Do we need to try to start blimp, if init failed? Done. https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... File blimp/client/android/java/src/org/chromium/blimp/BlimpRendererActivity.java (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:25: // LibraryLoader.get(LibraryProcessType.PROCESS_BROWSER).ensureInitialized(this); On 2015/08/19 07:48:07, nyquist (OOO - back 8-24) wrote: > Nit: I guess this is supposed to be removed? Done. https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:28: Log.e(TAG, "Native startup exception"); On 2015/08/19 07:48:07, nyquist (OOO - back 8-24) wrote: > Nit: Would we want to include the exception here? I don't think it should > normally cause an exception, so it should only be logged if there in fact is an > issue. I don't know what you mean :(. But I'm happy to change it. Let's talk tomorrow when you're back and you can explain it to me :). https://chromiumcodereview.appspot.com/1295243003/diff/1/ui/gfx/gfx.gyp File ui/gfx/gfx.gyp (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/ui/gfx/gfx.gyp#newco... ui/gfx/gfx.gyp:105: # 'test/gfx_util.h', On 2015/08/19 07:48:07, nyquist (OOO - back 8-24) wrote: > Why do we need to include a test file? Ah sorry none of this is necessary right now. My bad!
https://chromiumcodereview.appspot.com/1295243003/diff/20001/blimp/common/com... File blimp/common/compositor/blimp_layer_tree_settings.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/20001/blimp/common/com... blimp/common/compositor/blimp_layer_tree_settings.cc:71: void PopulateCommonLayerTreeSettings(cc::LayerTreeSettings& settings, I pulled most of this from RenderWidgetCompositor, which might not be the best idea in the future, but for now it seems to work :).
https://codereview.chromium.org/1295243003/diff/1/blimp/client/android/java/s... File blimp/client/android/java/src/org/chromium/blimp/BlimpRendererActivity.java (right): https://codereview.chromium.org/1295243003/diff/1/blimp/client/android/java/s... blimp/client/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:28: Log.e(TAG, "Native startup exception"); On 2015/08/21 00:49:46, David Trainor wrote: > On 2015/08/19 07:48:07, nyquist (OOO - back 8-24) wrote: > > Nit: Would we want to include the exception here? I don't think it should > > normally cause an exception, so it should only be logged if there in fact is > an > > issue. > > I don't know what you mean :(. But I'm happy to change it. Let's talk tomorrow > when you're back and you can explain it to me :). Oh, I was just thinking whether we should log the exception or not :-)
https://chromiumcodereview.appspot.com/1295243003/diff/20001/blimp/client/com... File blimp/client/compositor/blimp_compositor_android.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/20001/blimp/client/com... blimp/client/compositor/blimp_compositor_android.cc:80: // Adjust for some resolutions that barely straddle an extra How about computing the size tweaks mathematically? It would work for new screen dimensions that might be added in the future. if (portrait_width_ % default tile size < kMinimumExtraWidthPixels) default_tile_size += (kMinimumExtraWidthPixels - (portrait_width_ % default_tile_size)) https://chromiumcodereview.appspot.com/1295243003/diff/20001/blimp/client/com... File blimp/client/compositor/blimp_context_provider.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/20001/blimp/client/com... blimp/client/compositor/blimp_context_provider.cc:70: Handle or CHECK if context_ is nullptr? Looks like context->Initialize can fail. https://chromiumcodereview.appspot.com/1295243003/diff/20001/blimp/client/com... blimp/client/compositor/blimp_context_provider.cc:101: capabilities_.gpu.image = true; // TROLOLO This comment isn't helping my understanding. ;) https://chromiumcodereview.appspot.com/1295243003/diff/20001/ui/gfx/gfx.gyp File ui/gfx/gfx.gyp (right): https://chromiumcodereview.appspot.com/1295243003/diff/20001/ui/gfx/gfx.gyp#n... ui/gfx/gfx.gyp:71: # { Just delete this? There's always the Git revision history if we need to recover this build target.
https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... File blimp/client/android/java/src/org/chromium/blimp/BlimpRendererActivity.java (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:28: Log.e(TAG, "Native startup exception"); On 2015/08/21 00:52:23, nyquist (OOO - back 8-24) wrote: > On 2015/08/21 00:49:46, David Trainor wrote: > > On 2015/08/19 07:48:07, nyquist (OOO - back 8-24) wrote: > > > Nit: Would we want to include the exception here? I don't think it should > > > normally cause an exception, so it should only be logged if there in fact is > > an > > > issue. > > > > I don't know what you mean :(. But I'm happy to change it. Let's talk > tomorrow > > when you're back and you can explain it to me :). > > Oh, I was just thinking whether we should log the exception or not :-) Done. https://chromiumcodereview.appspot.com/1295243003/diff/20001/ui/gfx/gfx.gyp File ui/gfx/gfx.gyp (right): https://chromiumcodereview.appspot.com/1295243003/diff/20001/ui/gfx/gfx.gyp#n... ui/gfx/gfx.gyp:71: # { On 2015/08/21 18:46:35, Kevin Marshall wrote: > Just delete this? There's always the Git revision history if we need to recover > this build target. Ah sorry I forgot to do this :(. Thanks!
dtrainor@chromium.org changed reviewers: + sievers@chromium.org
+sievers for review of compositor code and some owners requirements.
dtrainor@chromium.org changed reviewers: + danakj@chromium.org, jbudorick@chromium.org, reed@chromium.org
Hi all! PTAL thanks :). This patch lands a basic Android 'compositor shell' that spins up a compositor instance without content/ or chrome/. From this we'll start building up the Blimp client and start iterating on the CC changes. sievers: - OWNERS for gpu and ui/gl DEPS. - General review for how I'm spinning up the Android compositor for a standalone SurfaceView (pulled some details from android_compositor_impl). danakj: - OWNERS for base, cc, and ui/gfx DEPS. reed: - OWNERS for skia and third_party/skia DEPS. jbudorick: - OWNERS for all.gyp
khushalsagar@chromium.org changed reviewers: + khushalsagar@chromium.org
https://codereview.chromium.org/1295243003/diff/100001/blimp/blimp.gyp File blimp/blimp.gyp (right): https://codereview.chromium.org/1295243003/diff/100001/blimp/blimp.gyp#newcode9 blimp/blimp.gyp:9: 'type': 'shared_library', The target should probably be component. We use the COMPONENT_BUILD variable in blimp_common_export.h which is defined based on the value of component. https://codereview.chromium.org/1295243003/diff/100001/blimp/common/blimp_com... File blimp/common/blimp_common_export.h (right): https://codereview.chromium.org/1295243003/diff/100001/blimp/common/blimp_com... blimp/common/blimp_common_export.h:23: #endif // defined(WIN32) nit: comment not needed here. https://codereview.chromium.org/1295243003/diff/100001/blimp/common/blimp_com... blimp/common/blimp_common_export.h:27: #endif // // defined(COMPONENT_BUILD) nit: not needed here.
https://codereview.chromium.org/1295243003/diff/100001/blimp/blimp.gyp File blimp/blimp.gyp (right): https://codereview.chromium.org/1295243003/diff/100001/blimp/blimp.gyp#newcode9 blimp/blimp.gyp:9: 'type': 'shared_library', On 2015/08/26 03:30:24, Khushal wrote: > The target should probably be component. We use the COMPONENT_BUILD variable in > blimp_common_export.h which is defined based on the value of component. Ah thanks for tracking that down! I'll change these targets.
> BUG= Please file at least one bug for tracking this work and point to it.
danakj@chromium.org changed reviewers: + enne@chromium.org
+enne for cc
https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/DEPS File blimp/client/DEPS (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/DE... blimp/client/DEPS:3: "+cc", also -cc/blink https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/DE... blimp/client/DEPS:10: "+ui/gfx", do you need all of gfx/ or just geometry/?
You may want to consider adding a PRESUBMIT.py now that does things like enforce linter correctness etc (see cc/PRESUBMIT.py for examples). It's much easier to avoid problems than fix them later. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/common/co... File blimp/common/compositor/blimp_layer_tree_settings.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/common/co... blimp/common/compositor/blimp_layer_tree_settings.h:16: } // namespace cc nit: normally don't use the // namespace comment when the block is so small and just forward decls
https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/common/DEPS File blimp/common/DEPS (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/common/DE... blimp/common/DEPS:3: "+cc", -cc/blink here too https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/common/DE... blimp/common/DEPS:7: "+ui/gfx", same question
GN? https://codereview.chromium.org/1295243003/diff/100001/blimp/blimp.gyp File blimp/blimp.gyp (right): https://codereview.chromium.org/1295243003/diff/100001/blimp/blimp.gyp#newcode14 blimp/blimp.gyp:14: '<(DEPTH)/skia/skia.gyp:skia', nit: ordering https://codereview.chromium.org/1295243003/diff/100001/blimp/blimp.gyp#newcode27 blimp/blimp.gyp:27: # TODO(dtrainor): For CC settings. This should be eventually removed. ...eventually?
On Wed, Aug 26, 2015 at 11:26 AM, <jbudorick@chromium.org> wrote: > GN? Oh yes. You might consider not even bothering to add gyp. That's what mojo's done. > > > > https://codereview.chromium.org/1295243003/diff/100001/blimp/blimp.gyp > File blimp/blimp.gyp (right): > > > https://codereview.chromium.org/1295243003/diff/100001/blimp/blimp.gyp#newcode14 > blimp/blimp.gyp:14: '<(DEPTH)/skia/skia.gyp:skia', > nit: ordering > > > https://codereview.chromium.org/1295243003/diff/100001/blimp/blimp.gyp#newcode27 > blimp/blimp.gyp:27: # TODO(dtrainor): For CC settings. This should be > eventually removed. > ...eventually? > > https://codereview.chromium.org/1295243003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Sorry; taking me a while to work through this one - here are comments up to the Android compositor impl. Disclaimer: This is my first Java code-review in Chromium, so some of these comments/questions probably make no sense... ;) https://codereview.chromium.org/1295243003/diff/1/blimp/blimp.gyp File blimp/blimp.gyp (right): https://codereview.chromium.org/1295243003/diff/1/blimp/blimp.gyp#newcode76 blimp/blimp.gyp:76: 'apk_name': 'Blimp', nit: Do we want the APK name capitalized in this way? Is there a convention requiring it, for example? https://codereview.chromium.org/1295243003/diff/1/blimp/client/DEPS File blimp/client/DEPS (right): https://codereview.chromium.org/1295243003/diff/1/blimp/client/DEPS#newcode4 blimp/client/DEPS:4: "+content/public/common", On 2015/08/19 07:48:06, nyquist (OOO - back 8-24) wrote: > Could we have > "-content", > "!content/public/common/content_switches.h" instead? I think I would find it > helpful if we instead clearly disallowed content with -content and then the > temporary rule for the switches until we remove that dependency. > > Or do you think we will end up needing a lot of //content? Feels cleanest to disallow content & then allow specific bits that we have ugly deps on - my 2c. https://codereview.chromium.org/1295243003/diff/1/blimp/client/android/blimp_... File blimp/client/android/blimp_jni_registrar.cc (right): https://codereview.chromium.org/1295243003/diff/1/blimp/client/android/blimp_... blimp/client/android/blimp_jni_registrar.cc:13: static base::android::RegistrationMethod kBlimpRegistrationMethods[] = { const? Why does this need to be static if it's already in an anon namespace? Could the constants be moved into RegisterBlimpJni, since they're only used there? https://codereview.chromium.org/1295243003/diff/1/blimp/client/android/blimp_... File blimp/client/android/blimp_jni_registrar.h (right): https://codereview.chromium.org/1295243003/diff/1/blimp/client/android/blimp_... blimp/client/android/blimp_jni_registrar.h:12: bool RegisterBlimpJni(JNIEnv* env); nit: Comment to explain what this does, who owns |env| & why it's req'd, what the return value means? e.g. does a return value of false mean All Is Lost...? https://codereview.chromium.org/1295243003/diff/1/blimp/client/android/blimp_... File blimp/client/android/blimp_library_loader.cc (right): https://codereview.chromium.org/1295243003/diff/1/blimp/client/android/blimp_... blimp/client/android/blimp_library_loader.cc:25: bool LibraryLoaded(JNIEnv* env, jclass clazz) { nit: OnLibraryLoaded https://codereview.chromium.org/1295243003/diff/1/blimp/client/android/blimp_... blimp/client/android/blimp_library_loader.cc:64: base::MessageLoopForUI::current()->Start(); So.. this is some fun hack around us not actually having control of the process' main thread? https://codereview.chromium.org/1295243003/diff/1/blimp/client/android/blimp_... File blimp/client/android/blimp_library_loader.h (right): https://codereview.chromium.org/1295243003/diff/1/blimp/client/android/blimp_... blimp/client/android/blimp_library_loader.h:24: #include <jni.h> On 2015/08/19 07:49:37, David Trainor wrote: > On 2015/08/19 07:48:06, nyquist wrote: > > Nit: This might not be enough. Add a couple more maybe? > > WTF vim ftl :(. nyquist@ I'd guess this is just dtrainor@ trying to unit-test our header guards? https://codereview.chromium.org/1295243003/diff/1/blimp/client/android/blimp_... blimp/client/android/blimp_library_loader.h:28: bool RegisterBlimpLibraryLoaderJni(JNIEnv* env); nit: As elsewhere, one-liner to clarify purpose, parameter & result meanings, etc. https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/b... File blimp/client/android/blimp_library_loader.cc (right): https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/b... blimp/client/android/blimp_library_loader.cc:25: bool LibraryLoaded(JNIEnv* env, jclass clazz) { nit: OnLibrariesLoaded (to fit the description of this callback's function) https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/b... blimp/client/android/blimp_library_loader.cc:29: // To view log output with IDs and timestamps use "adb logcat -v threadtime". nit: Prefer to have a blank line before comments, so that they're visually part of the relevant code block that follows them. nit: This comment seems to describe usage, rather than what the code below is doing - it should really be something like "Disable process info prefixes on log lines. These can be obtained via adb logcat -v threadtime if required." https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/b... blimp/client/android/blimp_library_loader.cc:34: VLOG(0) << "Chromium logging enabled: level = " << logging::GetMinLogLevel() nit: is "chromium" useful info here? is logging the log-level useful? seems a bit... meta? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/b... blimp/client/android/blimp_library_loader.cc:40: bool InitBlimp() { Can we name this something more helpful, e.g. OnJniInitializationComplete, or whatever it is that it is here for? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/b... blimp/client/android/blimp_library_loader.cc:49: static jboolean InitializeBlimp(JNIEnv* env, jclass clazz, jobject jcontext) { Let's not have InitBlimp _and_ InitializeBlimp here! Where is this function even called from...? Doesn't seem to be declared in the .h and it's not called from this code object..? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/b... blimp/client/android/blimp_library_loader.cc:57: static jboolean StartBlimp(JNIEnv* env, jclass clazz) { This doesn't seem to be called from anywhere, nor passed as a callback anywhere? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/b... blimp/client/android/blimp_library_loader.cc:83: !base::android::OnJNIOnLoadInit(init_callbacks)) { This OnJNIOnLoadInit() stuff is strangely obfuscating in the case where you only have a single initialization function. We could simply call it with no init callback, and continue our initialization if it returns true, and the code would be simpler, surely? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/b... File blimp/client/android/blimp_view.cc (right): https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/b... blimp/client/android/blimp_view.cc:14: static jlong Init(JNIEnv* env, Where is this called from? Can we give it a more helpful name? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/b... blimp/client/android/blimp_view.cc:44: compositor_.reset(); Why do you need to reset explicitly here, rather than letting implicit dtor take care of it? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/b... blimp/client/android/blimp_view.cc:68: current_surface_format_ = 0; nit: What does surface format of zero mean? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/b... File blimp/client/android/blimp_view.h (right): https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/b... blimp/client/android/blimp_view.h:31: float dp_to_pixel); nit: Document parameters. https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/b... blimp/client/android/blimp_view.h:33: // JNI Methods nit: Suggest "Handlers for calls from Java." or similar, so it's clear that these are Java->C++, not wrappers for C++->Java? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/b... blimp/client/android/blimp_view.h:34: void Destroy(JNIEnv* env, jobject jobj); nit: OnDestroy? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/b... blimp/client/android/blimp_view.h:36: void SurfaceChanged(JNIEnv* env, nit: OnSurfaceChanged, and similarly below? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/b... blimp/client/android/blimp_view.h:49: base::android::ScopedJavaGlobalRef<jobject> java_obj_; nit: Document this and |current_surface_format_| https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/b... blimp/client/android/blimp_view.h:52: int current_surface_format_; nit: You can initialize this in-line. https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/j... File blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java (right): https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/j... blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java:17: * A simple utility class to asynchronously load and register the native libraries associated with Given how awful this looks in codereview, and that style guide allows either 80 or 100 columns for Java, how would you feel about keeping Blimp .java to 80 columns? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/j... blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java:61: boolean startResult = initResult ? nativeStartBlimp() : false; n00b: Can this not be just initResult && nativeStartBlimp()...? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/j... blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java:62: if (callback != null) callback.onStartupComplete(startResult); nit: This if() needs braces according to style-guide. https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/j... File blimp/client/android/java/src/org/chromium/blimp/BlimpView.java (right): https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/j... blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:24: private long mNativeBlimpViewPtr; n00b: Is mFoo the preferred form for member variable names? Can't see any mention in style guide, either way? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/j... blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:36: protected void onFinishInflate() { Add a comment to clarify which interface this is overriding? Move the declaration down to the relevant section below? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/j... blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:82: if (mNativeBlimpViewPtr == 0) return; nit: Braces, here and below https://codereview.chromium.org/1295243003/diff/100001/blimp/client/android/j... blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:95: // SurfaceHolder.Callback2 Implementation ------------------------------------------------------ n00b nit: I thought this trailing --- thing was frowned upon, though I can't find anything prohibiting it... but... do we haz to? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... File blimp/client/compositor/blimp_compositor.cc (right): https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... blimp/client/compositor/blimp_compositor.cc:33: namespace { Why does this need a separate anonymous namespace? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... blimp/client/compositor/blimp_compositor.cc:44: // Make sure things get freed in the right order. nit: This comment explains why we're doing it explicitly, but not the specific constraints. https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... blimp/client/compositor/blimp_compositor.cc:58: // Create the LayerTreeHost nit: Blank line before block comment? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... blimp/client/compositor/blimp_compositor.cc:62: params.gpu_memory_buffer_manager = nullptr; nit: These two = nullptrs are redundant. https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... blimp/client/compositor/blimp_compositor.cc:76: // Build the root Layer nit: Punctuation - missing . https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... blimp/client/compositor/blimp_compositor.cc:84: // Destroy the LayerTreeHost nit: Punctuation Also, this comment seems redundant - it'd be more helpful to explain to the reader why that's all that we need to do to become invisible? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... blimp/client/compositor/blimp_compositor.cc:107: host_->SetOutputSurface(output_surface.Pass()); nit: Do you need the output_surface temporary? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... blimp/client/compositor/blimp_compositor.cc:133: if (!compositor_thread_) { nit: Suggest restructing this as an early-return if it's already set. https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... blimp/client/compositor/blimp_compositor.cc:146: false)); What's all this about? Surely you can just create the thread with a normal messgae-loop and IO won't be allowed on it? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... File blimp/client/compositor/blimp_compositor.h (right): https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... blimp/client/compositor/blimp_compositor.h:42: // LayerTreeHostClient Implementation ---------------------------------------- n00b nit: See my comment in the .java re trailing ---- nit: Elsewhere in Chromium we have comments like: // WibbleFlanger interface. rather than "implementation" - since this isn't actually the implementation. https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... blimp/client/compositor/blimp_compositor.h:43: void WillBeginMainFrame() override {} Chromium style guide requires that virtual functions _never_ be defined in-line. https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... blimp/client/compositor/blimp_compositor.h:75: // Used by this class when building the OutputSurface that this compositor "by this class" seems redundant? Suggest wording like: "Returns a widget for use in constructing this compositor's OutputSurface. Will only ever be called while this compositor is visible." https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... blimp/client/compositor/blimp_compositor.h:82: virtual void GenerateLayerTreeSettings(cc::LayerTreeSettings& settings, If this is an out-parameter then it should be cc::LayerTreeSettings*, not &. I'm unclear on what cc::LayerTreeSettings intended usage is; it is declared as a class but in all respects looks like a struct. All of it's members are PoD or PoD-equivalent AFAICT, so can you just have this be: virtual cc::LayerTreeSettings GenerateLayerTreeSettings(const base::CommandLine& cmd); https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... blimp/client/compositor/blimp_compositor.h:86: scoped_refptr<base::SingleThreadTaskRunner> GetCompositorTaskRunner(); nit: Comment to clarify what this does - what is the compositor task runner? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... blimp/client/compositor/blimp_compositor.h:88: gfx::Size viewport_size_; nit: Comments to explain the less obvious members, e.g. device_scale_factor_, host_ and compositor_thread_? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... blimp/client/compositor/blimp_compositor.h:91: scoped_ptr<cc::LayerTreeSettings> settings_; nit: Does this need to be a scoped_ptr<>, rather than a member? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... File blimp/client/compositor/blimp_compositor_android.cc (right): https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... blimp/client/compositor/blimp_compositor_android.cc:22: if (device_size.width() == 0 || device_size.height() == 0) { Isn't that device_size.IsEmpty()? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... File blimp/client/compositor/blimp_compositor_android.h (right): https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... blimp/client/compositor/blimp_compositor_android.h:32: float device_scale_factor); nit: Comment to explain the parameters https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... blimp/client/compositor/blimp_compositor_android.h:36: void SetSurface(JNIEnv* env, jobject jsurface); nit: Comment to explain e.g. ownership semantics for jsurface. Incidentally, why jsurface, not just surface? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... blimp/client/compositor/blimp_compositor_android.h:41: float device_scale_factor); nit: Comment parameters? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... blimp/client/compositor/blimp_compositor_android.h:51: bool real_size_supported_; nit: Comments on members' usage? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... blimp/client/compositor/blimp_compositor_android.h:53: gfx::AcceleratedWidget window_; nit: Initialize in-line? https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... File blimp/client/compositor/blimp_context_provider.h (right): https://codereview.chromium.org/1295243003/diff/100001/blimp/client/composito... blimp/client/compositor/blimp_context_provider.h:23: static scoped_refptr<BlimpContextProvider> Create( n00b: cc::ContextProvider is ref-counted? *sob*
https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... File blimp/client/compositor/blimp_compositor.h (right): https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... blimp/client/compositor/blimp_compositor.h:94: base::WeakPtrFactory<BlimpCompositor> weak_factory_; This seems to be unused. https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... File blimp/client/compositor/blimp_context_provider.cc (right): https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... blimp/client/compositor/blimp_context_provider.cc:51: DCHECK(main_thread_checker_.CalledOnValidThread()); Do you mean to DCHECK this here? We've only just constructed the BlimpContextProvider, so surely this thread-checker is already bound to this thread...? https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... blimp/client/compositor/blimp_context_provider.cc:54: gpu::gles2::ContextCreationAttribHelper attribs_for_gles2; Consider adding a comment to describe any specific properties of the context that we're creating here. https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... blimp/client/compositor/blimp_context_provider.cc:67: NULL /* share_context */, false /* share_resources */, attribs_for_gles2, nit: Why are these fields documented, but not the nullptr or false above, or below? https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... blimp/client/compositor/blimp_context_provider.cc:72: base::Bind(&BlimpContextProvider::OnLostContext, base::Unretained(this))); nit: Suggest moving this line up to be in a single block with the context_.reset(), for readability. https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... blimp/client/compositor/blimp_context_provider.cc:74: capabilities_.gpu.image = true; We do this in BindToCurrentThread - why do we want to do it here as well? https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... blimp/client/compositor/blimp_context_provider.cc:172: bool BlimpContextProvider::DestroyedOnMainThread() { Let's call out that this is the one and only override called on the main thread, in the header, setting it apart from the others, so it's not just mixed in with them. https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... File blimp/client/compositor/blimp_context_provider.h (right): https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... blimp/client/compositor/blimp_context_provider.h:23: static scoped_refptr<BlimpContextProvider> Create( Did I mention "oh no, ref-counting?!? so sad" yet? So sad. ;( https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... blimp/client/compositor/blimp_context_provider.h:26: void OnLostContext(); Who calls this, and why? What effect does it have? https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... blimp/client/compositor/blimp_context_provider.h:28: // cc::ContextProvider ------------------------------------------------------- See elsewhere re my nit-picky views on comment style. https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... blimp/client/compositor/blimp_context_provider.h:65: base::Lock context_lock_; Why is the context lock not grouped w/ the context? https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... File blimp/client/compositor/blimp_output_surface.cc (right): https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... blimp/client/compositor/blimp_output_surface.cc:24: client_->DidSwapBuffers(); nit: Comment to clarify what these calls are actually doing, e.g. presumably the first one actually swaps things and the second is some sort of feedback to an internal object of cc::OutputSurface? Why do we need to call DidSwapBuffers() anyway - why doesn't OutputSurface::OnSwapBuffersComplete do that? https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... blimp/client/compositor/blimp_output_surface.cc:26: base::MessageLoop::current()->task_runner()->PostTask( nit: Add a comment to indicate why we need to PostTask this, rather than calling immediately? https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... File blimp/client/compositor/blimp_output_surface.h (right): https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... blimp/client/compositor/blimp_output_surface.h:16: // cc::OutputSurface. nit: We can see it's a simple class, suggest reducing to "Minimal implementation of cc::OutputSurface." https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... blimp/client/compositor/blimp_output_surface.h:23: // OutputSurface Implementation ---------------------------------------------- nit: See my comment style whinges elsewhere. :) https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... File blimp/client/compositor/blimp_task_graph_runner.cc (right): https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... blimp/client/compositor/blimp_task_graph_runner.cc:23: cc::TaskGraphRunner::Run(); Oh, so ugly. ;( https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... File blimp/client/compositor/blimp_task_graph_runner.h (right): https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... blimp/client/compositor/blimp_task_graph_runner.h:15: // cc::TaskGraphRunner for details. nit: This comment can be simplified!
https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... File blimp/client/compositor/blimp_context_provider.h (right): https://codereview.chromium.org/1295243003/diff/140001/blimp/client/composito... blimp/client/compositor/blimp_context_provider.h:23: static scoped_refptr<BlimpContextProvider> Create( On 2015/08/27 18:23:55, Wez wrote: > Did I mention "oh no, ref-counting?!? so sad" yet? So sad. ;( Sorry :( The context provider can be held onto by the user of it after the context is lost until it receives a new context provider. This avoids a whole lot of plumbing ownerships around and lets the context factories make a new context in their callback about the context being lost rather than round-tripping through whoever has a context and making them also have "i dont have a context" potentially doubling the number of states in their code.
ok i think i addressed a bunch of the nits/commented on the ones i didn't. feel free to take another look thanks! https://chromiumcodereview.appspot.com/1295243003/diff/20001/blimp/client/com... File blimp/client/compositor/blimp_compositor_android.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/20001/blimp/client/com... blimp/client/compositor/blimp_compositor_android.cc:80: // Adjust for some resolutions that barely straddle an extra On 2015/08/21 18:46:35, Kevin Marshall wrote: > How about computing the size tweaks mathematically? It would work for new screen > dimensions that might be added in the future. > > if (portrait_width_ % default tile size < kMinimumExtraWidthPixels) > default_tile_size += (kMinimumExtraWidthPixels - (portrait_width_ % > default_tile_size)) Done. https://chromiumcodereview.appspot.com/1295243003/diff/20001/blimp/client/com... File blimp/client/compositor/blimp_context_provider.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/20001/blimp/client/com... blimp/client/compositor/blimp_context_provider.cc:70: On 2015/08/21 18:46:35, Kevin Marshall wrote: > Handle or CHECK if context_ is nullptr? Looks like context->Initialize can fail. Good call. Thanks! https://chromiumcodereview.appspot.com/1295243003/diff/20001/blimp/client/com... blimp/client/compositor/blimp_context_provider.cc:101: capabilities_.gpu.image = true; // TROLOLO On 2015/08/21 18:46:35, Kevin Marshall wrote: > This comment isn't helping my understanding. ;) lol I forgot about this! Ignore haha :). https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/blimp.gyp File blimp/blimp.gyp (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/blimp.gyp... blimp/blimp.gyp:14: '<(DEPTH)/skia/skia.gyp:skia', On 2015/08/26 18:26:12, jbudorick wrote: > nit: ordering Done. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/blimp.gyp... blimp/blimp.gyp:27: # TODO(dtrainor): For CC settings. This should be eventually removed. On 2015/08/26 18:26:12, jbudorick wrote: > ...eventually? Turned this into a TODO. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/DEPS File blimp/client/DEPS (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/DE... blimp/client/DEPS:3: "+cc", On 2015/08/26 18:21:16, danakj wrote: > also -cc/blink Done. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/DE... blimp/client/DEPS:10: "+ui/gfx", On 2015/08/26 18:21:16, danakj wrote: > do you need all of gfx/ or just geometry/? ui/gfx/buffer_types.h in common and ui/gfx/native_widget_types.h here. I explicitly added just those (let me know if you'd rather me just keep this as +ui/gfx). https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... File blimp/client/android/blimp_library_loader.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_library_loader.cc:25: bool LibraryLoaded(JNIEnv* env, jclass clazz) { On 2015/08/27 02:01:50, Wez wrote: > nit: OnLibrariesLoaded (to fit the description of this callback's function) Done. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_library_loader.cc:29: // To view log output with IDs and timestamps use "adb logcat -v threadtime". On 2015/08/27 02:01:50, Wez wrote: > nit: Prefer to have a blank line before comments, so that they're visually part > of the relevant code block that follows them. > > nit: This comment seems to describe usage, rather than what the code below is > doing - it should really be something like "Disable process info prefixes on log > lines. These can be obtained via adb logcat -v threadtime if required." Done. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_library_loader.cc:34: VLOG(0) << "Chromium logging enabled: level = " << logging::GetMinLogLevel() On 2015/08/27 02:01:50, Wez wrote: > nit: is "chromium" useful info here? is logging the log-level useful? seems a > bit... meta? I want to try to stay consistent with the rest of Chromium. If it isn't useful, we should just remove it from both here and the content/ executables. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_library_loader.cc:40: bool InitBlimp() { On 2015/08/27 02:01:50, Wez wrote: > Can we name this something more helpful, e.g. OnJniInitializationComplete, or > whatever it is that it is here for? Yeah that sounds cleaner. Changed. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_library_loader.cc:49: static jboolean InitializeBlimp(JNIEnv* env, jclass clazz, jobject jcontext) { On 2015/08/27 02:01:50, Wez wrote: > Let's not have InitBlimp _and_ InitializeBlimp here! > > Where is this function even called from...? Doesn't seem to be declared in the > .h and it's not called from this code object..? This is called from java (notice the JNIEnv*, and jclass, and jobject). The header file is auto-generated from the related java file and included here (see jni/BlimpLibraryLoader_jni.h). - If you see a "nativeXYZ()" in a Java file, it's backed by a native method. - If you see a native method with parameters "JNIEnv*, jclass, ..." it's a backing of a static Java method. - If you see a native method with parameters "JNIEnv*, jobject, ..." it's a backing of a non-static Java method. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_library_loader.cc:57: static jboolean StartBlimp(JNIEnv* env, jclass clazz) { On 2015/08/27 02:01:50, Wez wrote: > This doesn't seem to be called from anywhere, nor passed as a callback anywhere? See Java comment above. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_library_loader.cc:83: !base::android::OnJNIOnLoadInit(init_callbacks)) { On 2015/08/27 02:01:50, Wez wrote: > This OnJNIOnLoadInit() stuff is strangely obfuscating in the case where you only > have a single initialization function. We could simply call it with no init > callback, and continue our initialization if it returns true, and the code would > be simpler, surely? I'd rather not bypass the basic work flow (although I agree it ends up being a bit convoluted for this case)... if they ever refactor it I don't want this target to just be dropped on the floor. Other components (content/, chrome/, etc.) use this. We should do the same :). https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... File blimp/client/android/blimp_view.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_view.cc:14: static jlong Init(JNIEnv* env, On 2015/08/27 02:01:50, Wez wrote: > Where is this called from? Can we give it a more helpful name? From Java. I'd like to keep the same name. It's standard across the Chromium codebase (do a search for " Init(JNI"). This is backed by BlimpView#nativeInit(). The header declaration is in the auto-generated jni/BlimpView_jni.h. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_view.cc:44: compositor_.reset(); On 2015/08/27 02:01:50, Wez wrote: > Why do you need to reset explicitly here, rather than letting implicit dtor take > care of it? Ah I ended up refactoring some things so this might not matter anymore. There were some issues with the other Android compositor instances across our codebase where destruction order matters a lot. Will remove. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_view.cc:68: current_surface_format_ = 0; On 2015/08/27 02:01:50, Wez wrote: > nit: What does surface format of zero mean? In the Android SDK Java file it's defined as UNKNOWN. It's not explicitly defined in the native counterpart enum that the Android Java enum mirrors. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... File blimp/client/android/blimp_view.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_view.h:31: float dp_to_pixel); On 2015/08/27 02:01:50, Wez wrote: > nit: Document parameters. Done. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_view.h:33: // JNI Methods On 2015/08/27 02:01:51, Wez wrote: > nit: Suggest "Handlers for calls from Java." or similar, so it's clear that > these are Java->C++, not wrappers for C++->Java? Updated to "Methods called from Java via JNI" Generally C++ -> Java isn't exposed in any non-auto generated header. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_view.h:34: void Destroy(JNIEnv* env, jobject jobj); On 2015/08/27 02:01:50, Wez wrote: > nit: OnDestroy? I don't know if I agree. This actually destroys the object because it's owned by the Java component. Destroy() feels like a call that actually does the destruction. OnDestroy() feels like a notification call. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_view.h:36: void SurfaceChanged(JNIEnv* env, On 2015/08/27 02:01:51, Wez wrote: > nit: OnSurfaceChanged, and similarly below? Done. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_view.h:49: base::android::ScopedJavaGlobalRef<jobject> java_obj_; On 2015/08/27 02:01:50, Wez wrote: > nit: Document this and |current_surface_format_| Done. You'll see java_obj_ everywhere there's a native <-> JNI class coupling. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_view.h:52: int current_surface_format_; On 2015/08/27 02:01:50, Wez wrote: > nit: You can initialize this in-line. Oh I forgot about that! Is it required? IIRC the chromium-dev@ thread seemed to leave it open-ended. I'd rather initialize all variables together unless the new standard is to in-line initializations. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... File blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java:17: * A simple utility class to asynchronously load and register the native libraries associated with On 2015/08/27 02:01:51, Wez wrote: > Given how awful this looks in codereview, and that style guide allows either 80 > or 100 columns for Java, how would you feel about keeping Blimp .java to 80 > columns? Chromium java uses the Android style guide which explicitly states 100 characters (http://source.android.com/source/code-style.html#limit-line-length). I'd also rather stay consistent with the rest of Chromium. It would be somewhat awkward to expect other Chromium devs to know about/handle different line lengths for this directory vs. all others. Also updated the comment based on your other class comment suggestions :). https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java:61: boolean startResult = initResult ? nativeStartBlimp() : false; On 2015/08/27 02:01:51, Wez wrote: > n00b: Can this not be just initResult && nativeStartBlimp()...? Gah good point! https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java:62: if (callback != null) callback.onStartupComplete(startResult); On 2015/08/27 02:01:51, Wez wrote: > nit: This if() needs braces according to style-guide. Chromium Java uses the Android styleguide. http://source.android.com/source/code-style.html#use-standard-brace-style. Most of our Java code doesn't have {} for single line if's. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... File blimp/client/android/java/src/org/chromium/blimp/BlimpView.java (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:24: private long mNativeBlimpViewPtr; On 2015/08/27 02:01:51, Wez wrote: > n00b: Is mFoo the preferred form for member variable names? Can't see any > mention in style guide, either way? Ah yeah that's part of the Android style guide (http://source.android.com/source/code-style.html#follow-field-naming-conventions). https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:36: protected void onFinishInflate() { On 2015/08/27 02:01:51, Wez wrote: > Add a comment to clarify which interface this is overriding? Move the > declaration down to the relevant section below? It's overriding a base class method. I can move it down and add a section though. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:82: if (mNativeBlimpViewPtr == 0) return; On 2015/08/27 02:01:51, Wez wrote: > nit: Braces, here and below See java style guide comment above. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:95: // SurfaceHolder.Callback2 Implementation ------------------------------------------------------ On 2015/08/27 02:01:51, Wez wrote: > n00b nit: I thought this trailing --- thing was frowned upon, though I can't > find anything prohibiting it... but... do we haz to? I think it helps see a clear break in between sections, but I'm happy to remove it if it's frowned upon. Die ---! https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... File blimp/client/compositor/blimp_compositor.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.cc:33: namespace { On 2015/08/27 02:01:51, Wez wrote: > Why does this need a separate anonymous namespace? I wanted it separate so I could delete this chunk once we actually draw something in the compositor. This was just for testing purposes so something actually displays (makes sure the scheduler/threads/pipelines are working properly). I'll move it into the other namespace though :). https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.cc:44: // Make sure things get freed in the right order. On 2015/08/27 02:01:51, Wez wrote: > nit: This comment explains why we're doing it explicitly, but not the specific > constraints. Done. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.cc:58: // Create the LayerTreeHost On 2015/08/27 02:01:51, Wez wrote: > nit: Blank line before block comment? Done. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.cc:62: params.gpu_memory_buffer_manager = nullptr; On 2015/08/27 02:01:51, Wez wrote: > nit: These two = nullptrs are redundant. Done. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.cc:76: // Build the root Layer On 2015/08/27 02:01:51, Wez wrote: > nit: Punctuation - missing . Done. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.cc:84: // Destroy the LayerTreeHost On 2015/08/27 02:01:51, Wez wrote: > nit: Punctuation > > Also, this comment seems redundant - it'd be more helpful to explain to the > reader why that's all that we need to do to become invisible? Done. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.cc:107: host_->SetOutputSurface(output_surface.Pass()); On 2015/08/27 02:01:51, Wez wrote: > nit: Do you need the output_surface temporary? Done. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.cc:133: if (!compositor_thread_) { On 2015/08/27 02:01:51, Wez wrote: > nit: Suggest restructing this as an early-return if it's already set. Done. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.cc:146: false)); On 2015/08/27 02:01:51, Wez wrote: > What's all this about? Surely you can just create the thread with a normal > messgae-loop and IO won't be allowed on it? I pulled this piece from render_thread_impl.cc. Looking at thread_restrictions.cc, it looks like the lazy ThreadLocalBoolean is called g_io_disallowed (which defaults to false, which means IO is allowed). Yay double negatives? I think I have to do this to turn off IO on this thread. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... File blimp/client/compositor/blimp_compositor.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.h:42: // LayerTreeHostClient Implementation ---------------------------------------- On 2015/08/27 02:01:51, Wez wrote: > n00b nit: See my comment in the .java re trailing ---- > > nit: Elsewhere in Chromium we have comments like: > > // WibbleFlanger interface. > > rather than "implementation" - since this isn't actually the implementation. Be gone ---! I was looking at the cc files and they seem to do "<ClassName> implementation", which makes sense to me. This is the implementation though, right? We're implementating the interface here? https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.h:43: void WillBeginMainFrame() override {} On 2015/08/27 02:01:51, Wez wrote: > Chromium style guide requires that virtual functions _never_ be defined in-line. Ah my bad! I saw this in ui/compositor/compositor.h and compositor_impl_android.h when I was looking for the right way to implement this. Are these all incorrect? I can look into fixing those in another patch if so. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.h:75: // Used by this class when building the OutputSurface that this compositor On 2015/08/27 02:01:51, Wez wrote: > "by this class" seems redundant? > > Suggest wording like: "Returns a widget for use in constructing this > compositor's OutputSurface. Will only ever be called while this compositor is > visible." Much better thanks :). https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.h:82: virtual void GenerateLayerTreeSettings(cc::LayerTreeSettings& settings, On 2015/08/27 02:01:51, Wez wrote: > If this is an out-parameter then it should be cc::LayerTreeSettings*, not &. > > I'm unclear on what cc::LayerTreeSettings intended usage is; it is declared as a > class but in all respects looks like a struct. All of it's members are PoD or > PoD-equivalent AFAICT, so can you just have this be: > > virtual cc::LayerTreeSettings GenerateLayerTreeSettings(const base::CommandLine& > cmd); I changed it to LayerTreeSettings*. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.h:86: scoped_refptr<base::SingleThreadTaskRunner> GetCompositorTaskRunner(); On 2015/08/27 02:01:51, Wez wrote: > nit: Comment to clarify what this does - what is the compositor task runner? Done. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.h:88: gfx::Size viewport_size_; On 2015/08/27 02:01:51, Wez wrote: > nit: Comments to explain the less obvious members, e.g. device_scale_factor_, > host_ and compositor_thread_? Done. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.h:91: scoped_ptr<cc::LayerTreeSettings> settings_; On 2015/08/27 02:01:51, Wez wrote: > nit: Does this need to be a scoped_ptr<>, rather than a member? I wanted to easily know when it wasn't initialized yet (I thought about adding a boolean but this seemed easier). Like the other comment above about just returning the reference, if you think the copy constructor and rebuilding the settings when the app comes to the foreground costs are negligible I can make these changes. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... File blimp/client/compositor/blimp_compositor_android.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.cc:22: if (device_size.width() == 0 || device_size.height() == 0) { On 2015/08/27 02:01:52, Wez wrote: > Isn't that device_size.IsEmpty()? Ah better! :) https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... File blimp/client/compositor/blimp_compositor_android.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.h:32: float device_scale_factor); On 2015/08/27 02:01:52, Wez wrote: > nit: Comment to explain the parameters Done. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.h:36: void SetSurface(JNIEnv* env, jobject jsurface); On 2015/08/27 02:01:52, Wez wrote: > nit: Comment to explain e.g. ownership semantics for jsurface. > > Incidentally, why jsurface, not just surface? Done. I do jsurface because it's a Java object. This makes it easy when marshalling to native objects because the native version can have the same name - the j. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.h:41: float device_scale_factor); On 2015/08/27 02:01:52, Wez wrote: > nit: Comment parameters? Done. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.h:51: bool real_size_supported_; On 2015/08/27 02:01:52, Wez wrote: > nit: Comments on members' usage? Done. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.h:53: gfx::AcceleratedWidget window_; On 2015/08/27 02:01:52, Wez wrote: > nit: Initialize in-line? Still would prefer all constructor initialization, unless it's required :(. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... File blimp/client/compositor/blimp_context_provider.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_context_provider.h:23: static scoped_refptr<BlimpContextProvider> Create( On 2015/08/27 02:01:52, Wez wrote: > n00b: cc::ContextProvider is ref-counted? *sob* :'( https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/common/DEPS File blimp/common/DEPS (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/common/DE... blimp/common/DEPS:3: "+cc", On 2015/08/26 18:23:36, danakj wrote: > -cc/blink here too Done. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/common/DE... blimp/common/DEPS:7: "+ui/gfx", On 2015/08/26 18:23:35, danakj wrote: > same question Done. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/common/bl... File blimp/common/blimp_common_export.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/common/bl... blimp/common/blimp_common_export.h:23: #endif // defined(WIN32) On 2015/08/26 03:30:24, Khushal wrote: > nit: comment not needed here. Done. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/common/bl... blimp/common/blimp_common_export.h:27: #endif // // defined(COMPONENT_BUILD) On 2015/08/26 03:30:24, Khushal wrote: > nit: not needed here. Done. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/common/co... File blimp/common/compositor/blimp_layer_tree_settings.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/common/co... blimp/common/compositor/blimp_layer_tree_settings.h:16: } // namespace cc On 2015/08/26 18:23:05, danakj wrote: > nit: normally don't use the // namespace comment when the block is so small and > just forward decls Done. https://chromiumcodereview.appspot.com/1295243003/diff/140001/blimp/client/co... File blimp/client/compositor/blimp_compositor.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/140001/blimp/client/co... blimp/client/compositor/blimp_compositor.h:94: base::WeakPtrFactory<BlimpCompositor> weak_factory_; On 2015/08/27 18:23:54, Wez wrote: > This seems to be unused. Gah thanks! Forgot to take it out. https://chromiumcodereview.appspot.com/1295243003/diff/140001/blimp/client/co... File blimp/client/compositor/blimp_context_provider.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/140001/blimp/client/co... blimp/client/compositor/blimp_context_provider.cc:51: DCHECK(main_thread_checker_.CalledOnValidThread()); On 2015/08/27 18:23:55, Wez wrote: > Do you mean to DCHECK this here? We've only just constructed the > BlimpContextProvider, so surely this thread-checker is already bound to this > thread...? Done. https://chromiumcodereview.appspot.com/1295243003/diff/140001/blimp/client/co... blimp/client/compositor/blimp_context_provider.cc:54: gpu::gles2::ContextCreationAttribHelper attribs_for_gles2; On 2015/08/27 18:23:55, Wez wrote: > Consider adding a comment to describe any specific properties of the context > that we're creating here. Sorry I'm not quite sure what properties you want comments on. Do you mean the particular fields of the ContextCreationAttribHelper we're setting or do you mean more at a high level? https://chromiumcodereview.appspot.com/1295243003/diff/140001/blimp/client/co... blimp/client/compositor/blimp_context_provider.cc:67: NULL /* share_context */, false /* share_resources */, attribs_for_gles2, On 2015/08/27 18:23:55, Wez wrote: > nit: Why are these fields documented, but not the nullptr or false above, or > below? Done. https://chromiumcodereview.appspot.com/1295243003/diff/140001/blimp/client/co... blimp/client/compositor/blimp_context_provider.cc:72: base::Bind(&BlimpContextProvider::OnLostContext, base::Unretained(this))); On 2015/08/27 18:23:55, Wez wrote: > nit: Suggest moving this line up to be in a single block with the > context_.reset(), for readability. Done. https://chromiumcodereview.appspot.com/1295243003/diff/140001/blimp/client/co... blimp/client/compositor/blimp_context_provider.cc:74: capabilities_.gpu.image = true; On 2015/08/27 18:23:54, Wez wrote: > We do this in BindToCurrentThread - why do we want to do it here as well? Good point! I traced through the code and it should be fine (also the interface kind of makes it clear that calling BindToCurrentThread should be called before accessing the contexts). https://chromiumcodereview.appspot.com/1295243003/diff/140001/blimp/client/co... blimp/client/compositor/blimp_context_provider.cc:172: bool BlimpContextProvider::DestroyedOnMainThread() { On 2015/08/27 18:23:55, Wez wrote: > Let's call out that this is the one and only override called on the main thread, > in the header, setting it apart from the others, so it's not just mixed in with > them. I'm not sure I agree. That should be an interface-level comment (this is just the implementation that follows the set of rules specified by the interface). I think the name OnMainThread makes it explicit as well. https://chromiumcodereview.appspot.com/1295243003/diff/140001/blimp/client/co... File blimp/client/compositor/blimp_context_provider.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/140001/blimp/client/co... blimp/client/compositor/blimp_context_provider.h:26: void OnLostContext(); On 2015/08/27 18:23:55, Wez wrote: > Who calls this, and why? What effect does it have? Made it private. It's part of the flow for handling lost GL contexts (required by the interfaces). Basically we plumb this call through from GLInProcessContext to the callback specified by ContextProvider::SetLostCallback and clean up our internal state. I'll don't know all of the internal details, but I'll take a look. https://chromiumcodereview.appspot.com/1295243003/diff/140001/blimp/client/co... blimp/client/compositor/blimp_context_provider.h:28: // cc::ContextProvider ------------------------------------------------------- On 2015/08/27 18:23:55, Wez wrote: > See elsewhere re my nit-picky views on comment style. Done. https://chromiumcodereview.appspot.com/1295243003/diff/140001/blimp/client/co... blimp/client/compositor/blimp_context_provider.h:65: base::Lock context_lock_; On 2015/08/27 18:23:55, Wez wrote: > Why is the context lock not grouped w/ the context? It's only ever used by the cc::ContextProvider interface (not by this class). destroyed_lock_ is directly used with destroyed_ in this class, which is why those were grouped. Grouping it closer makes sense though :). https://chromiumcodereview.appspot.com/1295243003/diff/140001/blimp/client/co... File blimp/client/compositor/blimp_output_surface.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/140001/blimp/client/co... blimp/client/compositor/blimp_output_surface.cc:24: client_->DidSwapBuffers(); On 2015/08/27 18:23:55, Wez wrote: > nit: Comment to clarify what these calls are actually doing, e.g. presumably the > first one actually swaps things and the second is some sort of feedback to an > internal object of cc::OutputSurface? Why do we need to call DidSwapBuffers() > anyway - why doesn't OutputSurface::OnSwapBuffersComplete do that? The comment on the interface is pretty clear about what we have to do. I'm just implementing at per those instructions (one line of code - after the change below - per step in the interface comment). I'll add a "see cc::OutputSurface::SwapBuffers for details" if you'd like. https://chromiumcodereview.appspot.com/1295243003/diff/140001/blimp/client/co... blimp/client/compositor/blimp_output_surface.cc:26: base::MessageLoop::current()->task_runner()->PostTask( On 2015/08/27 18:23:55, Wez wrote: > nit: Add a comment to indicate why we need to PostTask this, rather than calling > immediately? It's the advice I got when discussing how to implement this. I'll investigate for details for why. Also changed this to just use cc::OutputSurface::PostSwapBuffersComplete. https://chromiumcodereview.appspot.com/1295243003/diff/140001/blimp/client/co... File blimp/client/compositor/blimp_output_surface.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/140001/blimp/client/co... blimp/client/compositor/blimp_output_surface.h:16: // cc::OutputSurface. On 2015/08/27 18:23:55, Wez wrote: > nit: We can see it's a simple class, suggest reducing to "Minimal implementation > of cc::OutputSurface." Done. https://chromiumcodereview.appspot.com/1295243003/diff/140001/blimp/client/co... blimp/client/compositor/blimp_output_surface.h:23: // OutputSurface Implementation ---------------------------------------------- On 2015/08/27 18:23:55, Wez wrote: > nit: See my comment style whinges elsewhere. :) Done. https://chromiumcodereview.appspot.com/1295243003/diff/140001/blimp/client/co... File blimp/client/compositor/blimp_task_graph_runner.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/140001/blimp/client/co... blimp/client/compositor/blimp_task_graph_runner.cc:23: cc::TaskGraphRunner::Run(); On 2015/08/27 18:23:55, Wez wrote: > Oh, so ugly. ;( Acknowledged. https://chromiumcodereview.appspot.com/1295243003/diff/140001/blimp/client/co... File blimp/client/compositor/blimp_task_graph_runner.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/140001/blimp/client/co... blimp/client/compositor/blimp_task_graph_runner.h:15: // cc::TaskGraphRunner for details. On 2015/08/27 18:23:55, Wez wrote: > nit: This comment can be simplified! Done.
I also removed the gyp files and target. I'll try just using gn (seemed to be wez@'s preference too and it looks like Android stuff builds with gn now :D!).
https://chromiumcodereview.appspot.com/1295243003/diff/180001/blimp/client/BU... File blimp/client/BUILD.gn (right): https://chromiumcodereview.appspot.com/1295243003/diff/180001/blimp/client/BU... blimp/client/BUILD.gn:25: "//blimp/common:blimp_common", Will fix ordering.
https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/DEPS File blimp/client/DEPS (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/DE... blimp/client/DEPS:10: "+ui/gfx", On 2015/08/28 01:23:44, David Trainor wrote: > On 2015/08/26 18:21:16, danakj wrote: > > do you need all of gfx/ or just geometry/? > > ui/gfx/buffer_types.h in common and ui/gfx/native_widget_types.h here. I > explicitly added just those (let me know if you'd rather me just keep this as > +ui/gfx). Ah, ok ui/gfx/ makes sense then. Thanks.
the ui/gfx/ and cc/ DEPS LGTM
https://chromiumcodereview.appspot.com/1295243003/diff/180001/blimp/client/co... File blimp/client/compositor/blimp_context_provider.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/180001/blimp/client/co... blimp/client/compositor/blimp_context_provider.cc:72: CHECK(context_); What's the point of this? You just crashed on the line above if context_ is null. (And why CHECK not DCHECK.)
https://chromiumcodereview.appspot.com/1295243003/diff/180001/blimp/client/BU... File blimp/client/BUILD.gn (right): https://chromiumcodereview.appspot.com/1295243003/diff/180001/blimp/client/BU... blimp/client/BUILD.gn:25: "//blimp/common:blimp_common", On 2015/08/28 10:09:33, David Trainor wrote: > Will fix ordering. Done. https://chromiumcodereview.appspot.com/1295243003/diff/180001/blimp/client/co... File blimp/client/compositor/blimp_context_provider.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/180001/blimp/client/co... blimp/client/compositor/blimp_context_provider.cc:72: CHECK(context_); On 2015/08/28 17:38:46, danakj wrote: > What's the point of this? You just crashed on the line above if context_ is > null. (And why CHECK not DCHECK.) Gah I put it in the wrong place, I meant to put it before the dereference. Also I just re-read the style guide on this and it should be a DCHECK.
https://chromiumcodereview.appspot.com/1295243003/diff/180001/blimp/client/co... File blimp/client/compositor/blimp_context_provider.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/180001/blimp/client/co... blimp/client/compositor/blimp_context_provider.cc:72: CHECK(context_); On 2015/08/28 18:24:16, David Trainor wrote: > On 2015/08/28 17:38:46, danakj wrote: > > What's the point of this? You just crashed on the line above if context_ is > > null. (And why CHECK not DCHECK.) > > Gah I put it in the wrong place, I meant to put it before the dereference. Also > I just re-read the style guide on this and it should be a DCHECK. Actually, if you're about to deref the variable, and therefor crash in a predictable way anyway, then you shouldn't put a [D]CHECK before it at all.
dtrainor@chromium.org changed reviewers: + reed@google.com
Ping! Adding reed@google.com because that's the actual owner (not reed@chromium.org). I'm adding skia and third_party/skia to blimp/client/DEPS and blimp/common/DEPS because I'm building a GrContext and using SkColor. Thanks!
https://chromiumcodereview.appspot.com/1295243003/diff/180001/blimp/client/co... File blimp/client/compositor/blimp_context_provider.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/180001/blimp/client/co... blimp/client/compositor/blimp_context_provider.cc:72: CHECK(context_); On 2015/08/31 16:27:18, Wez wrote: > On 2015/08/28 18:24:16, David Trainor wrote: > > On 2015/08/28 17:38:46, danakj wrote: > > > What's the point of this? You just crashed on the line above if context_ is > > > null. (And why CHECK not DCHECK.) > > > > Gah I put it in the wrong place, I meant to put it before the dereference. > Also > > I just re-read the style guide on this and it should be a DCHECK. > > Actually, if you're about to deref the variable, and therefor crash in a > predictable way anyway, then you shouldn't put a [D]CHECK before it at all. Yeah that's true. I can make that change here because this is an interface, but in general I'm a little wary of doing that because if the method doesn't access the vtable/any member variables it won't necessarily crash (undefined behavior). If it calls a method on a member that also doesn't access it's vtable it gets even more interesting. https://chromiumcodereview.appspot.com/1295243003/diff/180001/blimp/client/co... File blimp/client/compositor/blimp_context_provider.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/180001/blimp/client/co... blimp/client/compositor/blimp_context_provider.h:16: #include "ui/gl/gl_surface.h" Oops
https://chromiumcodereview.appspot.com/1295243003/diff/180001/blimp/client/co... File blimp/client/compositor/blimp_context_provider.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/180001/blimp/client/co... blimp/client/compositor/blimp_context_provider.cc:72: CHECK(context_); On 2015/08/31 21:25:43, David Trainor wrote: > On 2015/08/31 16:27:18, Wez wrote: > > On 2015/08/28 18:24:16, David Trainor wrote: > > > On 2015/08/28 17:38:46, danakj wrote: > > > > What's the point of this? You just crashed on the line above if context_ > is > > > > null. (And why CHECK not DCHECK.) > > > > > > Gah I put it in the wrong place, I meant to put it before the dereference. > > Also > > > I just re-read the style guide on this and it should be a DCHECK. > > > > Actually, if you're about to deref the variable, and therefor crash in a > > predictable way anyway, then you shouldn't put a [D]CHECK before it at all. > > Yeah that's true. I can make that change here because this is an interface, but > in general I'm a little wary of doing that because if the method doesn't access > the vtable/any member variables it won't necessarily crash (undefined behavior). > If it calls a method on a member that also doesn't access it's vtable it gets > even more interesting. scoped_ptr does have an assert, but it looks like that just becomes LOG_ASSERT which doesn't actually stop execution if it fails on debug builds? Either that or I'm misreading it :(.
On Mon, Aug 31, 2015 at 2:27 PM, <dtrainor@chromium.org> wrote: > > > https://chromiumcodereview.appspot.com/1295243003/diff/180001/blimp/client/co... > File blimp/client/compositor/blimp_context_provider.cc (right): > > > https://chromiumcodereview.appspot.com/1295243003/diff/180001/blimp/client/co... > blimp/client/compositor/blimp_context_provider.cc:72: CHECK(context_); > On 2015/08/31 21:25:43, David Trainor wrote: > >> On 2015/08/31 16:27:18, Wez wrote: >> > On 2015/08/28 18:24:16, David Trainor wrote: >> > > On 2015/08/28 17:38:46, danakj wrote: >> > > > What's the point of this? You just crashed on the line above if >> > context_ > >> is >> > > > null. (And why CHECK not DCHECK.) >> > > >> > > Gah I put it in the wrong place, I meant to put it before the >> > dereference. > >> > Also >> > > I just re-read the style guide on this and it should be a DCHECK. >> > >> > Actually, if you're about to deref the variable, and therefor crash >> > in a > >> > predictable way anyway, then you shouldn't put a [D]CHECK before it >> > at all. > > Yeah that's true. I can make that change here because this is an >> > interface, but > >> in general I'm a little wary of doing that because if the method >> > doesn't access > >> the vtable/any member variables it won't necessarily crash (undefined >> > behavior). > >> If it calls a method on a member that also doesn't access it's vtable >> > it gets > >> even more interesting. >> > > scoped_ptr does have an assert, but it looks like that just becomes > LOG_ASSERT which doesn't actually stop execution if it fails on debug > builds? Either that or I'm misreading it :(. > Where do you see that assert becomes LOG_ASSERT? assert is http://en.cppreference.com/w/cpp/error/assert which crashes in debug builds. You could try it and see. > > https://chromiumcodereview.appspot.com/1295243003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/08/31 21:30:08, danakj wrote: > On Mon, Aug 31, 2015 at 2:27 PM, <mailto:dtrainor@chromium.org> wrote: > > > > > > > > https://chromiumcodereview.appspot.com/1295243003/diff/180001/blimp/client/co... > > File blimp/client/compositor/blimp_context_provider.cc (right): > > > > > > > https://chromiumcodereview.appspot.com/1295243003/diff/180001/blimp/client/co... > > blimp/client/compositor/blimp_context_provider.cc:72: CHECK(context_); > > On 2015/08/31 21:25:43, David Trainor wrote: > > > >> On 2015/08/31 16:27:18, Wez wrote: > >> > On 2015/08/28 18:24:16, David Trainor wrote: > >> > > On 2015/08/28 17:38:46, danakj wrote: > >> > > > What's the point of this? You just crashed on the line above if > >> > > context_ > > > >> is > >> > > > null. (And why CHECK not DCHECK.) > >> > > > >> > > Gah I put it in the wrong place, I meant to put it before the > >> > > dereference. > > > >> > Also > >> > > I just re-read the style guide on this and it should be a DCHECK. > >> > > >> > Actually, if you're about to deref the variable, and therefor crash > >> > > in a > > > >> > predictable way anyway, then you shouldn't put a [D]CHECK before it > >> > > at all. > > > > Yeah that's true. I can make that change here because this is an > >> > > interface, but > > > >> in general I'm a little wary of doing that because if the method > >> > > doesn't access > > > >> the vtable/any member variables it won't necessarily crash (undefined > >> > > behavior). > > > >> If it calls a method on a member that also doesn't access it's vtable > >> > > it gets > > > >> even more interesting. > >> > > > > scoped_ptr does have an assert, but it looks like that just becomes > > LOG_ASSERT which doesn't actually stop execution if it fails on debug > > builds? Either that or I'm misreading it :(. > > > > Where do you see that assert becomes LOG_ASSERT? assert is > http://en.cppreference.com/w/cpp/error/assert which crashes in debug > builds. You could try it and see. > Ah yay it does :). F3 on eclipse jumped me to logging.h, which had the following: // Redefine the standard assert to use our nice log files #undef assert #define assert(x) DLOG_ASSERT(x) > > > > > https://chromiumcodereview.appspot.com/1295243003/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On Mon, Aug 31, 2015 at 3:28 PM, <dtrainor@chromium.org> wrote: > On 2015/08/31 21:30:08, danakj wrote: > > On Mon, Aug 31, 2015 at 2:27 PM, <mailto:dtrainor@chromium.org> wrote: >> > > > >> > >> > >> > > > https://chromiumcodereview.appspot.com/1295243003/diff/180001/blimp/client/co... > >> > File blimp/client/compositor/blimp_context_provider.cc (right): >> > >> > >> > >> > > > https://chromiumcodereview.appspot.com/1295243003/diff/180001/blimp/client/co... > >> > blimp/client/compositor/blimp_context_provider.cc:72: CHECK(context_); >> > On 2015/08/31 21:25:43, David Trainor wrote: >> > >> >> On 2015/08/31 16:27:18, Wez wrote: >> >> > On 2015/08/28 18:24:16, David Trainor wrote: >> >> > > On 2015/08/28 17:38:46, danakj wrote: >> >> > > > What's the point of this? You just crashed on the line above if >> >> >> > context_ >> > >> >> is >> >> > > > null. (And why CHECK not DCHECK.) >> >> > > >> >> > > Gah I put it in the wrong place, I meant to put it before the >> >> >> > dereference. >> > >> >> > Also >> >> > > I just re-read the style guide on this and it should be a DCHECK. >> >> > >> >> > Actually, if you're about to deref the variable, and therefor crash >> >> >> > in a >> > >> >> > predictable way anyway, then you shouldn't put a [D]CHECK before it >> >> >> > at all. >> > >> > Yeah that's true. I can make that change here because this is an >> >> >> > interface, but >> > >> >> in general I'm a little wary of doing that because if the method >> >> >> > doesn't access >> > >> >> the vtable/any member variables it won't necessarily crash (undefined >> >> >> > behavior). >> > >> >> If it calls a method on a member that also doesn't access it's vtable >> >> >> > it gets >> > >> >> even more interesting. >> >> >> > >> > scoped_ptr does have an assert, but it looks like that just becomes >> > LOG_ASSERT which doesn't actually stop execution if it fails on debug >> > builds? Either that or I'm misreading it :(. >> > >> > > Where do you see that assert becomes LOG_ASSERT? assert is >> http://en.cppreference.com/w/cpp/error/assert which crashes in debug >> builds. You could try it and see. >> > > Ah yay it does :). F3 on eclipse jumped me to logging.h, which had the > following: > > // Redefine the standard assert to use our nice log files > #undef assert > #define assert(x) DLOG_ASSERT(x) > oic :O thanks. in this case it doesn't include base/logging.h or it would have used DCHECK I guess :) > > > > >> > https://chromiumcodereview.appspot.com/1295243003/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > > https://chromiumcodereview.appspot.com/1295243003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm https://chromiumcodereview.appspot.com/1295243003/diff/260001/blimp/client/an... File blimp/client/android/blimp_library_loader.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/260001/blimp/client/an... blimp/client/android/blimp_library_loader.cc:76: g_main_message_loop.Get().reset(new base::MessageLoopForUI); Where is the AtExitManager destroyed that would delete the scoped_ptr<MsgLoop> (because it's a LazyInstance)? https://chromiumcodereview.appspot.com/1295243003/diff/260001/blimp/client/an... File blimp/client/android/java/src/org/chromium/blimp/BlimpView.java (right): https://chromiumcodereview.appspot.com/1295243003/diff/260001/blimp/client/an... blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:53: physicalSize.x, physicalSize.y, displaySize.x, displaySize.y, compositorDensity); so does that work if physicalSize wasn't set? https://chromiumcodereview.appspot.com/1295243003/diff/260001/blimp/client/co... File blimp/client/compositor/blimp_compositor.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/260001/blimp/client/co... blimp/client/compositor/blimp_compositor.h:11: #include "base/memory/weak_ptr.h" are refcounted and weakptr unused?
https://chromiumcodereview.appspot.com/1295243003/diff/260001/blimp/client/an... File blimp/client/android/blimp_library_loader.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/260001/blimp/client/an... blimp/client/android/blimp_library_loader.cc:76: g_main_message_loop.Get().reset(new base::MessageLoopForUI); On 2015/08/31 23:50:33, sievers wrote: > Where is the AtExitManager destroyed that would delete the scoped_ptr<MsgLoop> > (because it's a LazyInstance)? I think we're good: JNI_OnLoad -> base::android::OnJNIOnLoadInit -> base::android::Init (see base_jni_onload.cc) -> InitAtExitManager. Although to be honest, I'm not exactly sure what calls LibraryLoaderExitHook to actually delete the AtExitManager. Looking into it. https://chromiumcodereview.appspot.com/1295243003/diff/260001/blimp/client/an... File blimp/client/android/java/src/org/chromium/blimp/BlimpView.java (right): https://chromiumcodereview.appspot.com/1295243003/diff/260001/blimp/client/an... blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:53: physicalSize.x, physicalSize.y, displaySize.x, displaySize.y, compositorDensity); On 2015/08/31 23:50:33, sievers wrote: > so does that work if physicalSize wasn't set? Yeah. These are just used to determine the optimal tile size. https://chromiumcodereview.appspot.com/1295243003/diff/260001/blimp/client/co... File blimp/client/compositor/blimp_compositor.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/260001/blimp/client/co... blimp/client/compositor/blimp_compositor.h:11: #include "base/memory/weak_ptr.h" On 2015/08/31 23:50:34, sievers wrote: > are refcounted and weakptr unused? refcounted is used by GetCompositorTaskRunner(), but weakptr isn't good point.
dtrainor@chromium.org changed reviewers: + dpranke@chromium.org
dpranke@: Can I get an OWNER review of the top level BUILD.gn? Thanks!
lgtm
lgtm
David, can you update the CL description to indicate which pre-existing Chromium components this impl was based upon, so that if there are any questions later about why certain inherited design decisions were made, it's at least possible to do the detective work to find out?
On 2015/09/02 23:26:58, Wez wrote: > David, can you update the CL description to indicate which pre-existing Chromium > components this impl was based upon, so that if there are any questions later > about why certain inherited design decisions were made, it's at least possible > to do the detective work to find out? Sure will do. Good idea :).
Almost there; mostly comment-related nits here. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... File blimp/client/android/blimp_library_loader.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_library_loader.cc:34: VLOG(0) << "Chromium logging enabled: level = " << logging::GetMinLogLevel() On 2015/08/28 01:23:45, David Trainor wrote: > On 2015/08/27 02:01:50, Wez wrote: > > nit: is "chromium" useful info here? is logging the log-level useful? seems a > > bit... meta? > > I want to try to stay consistent with the rest of Chromium. If it isn't useful, > we should just remove it from both here and the content/ executables. Acknowledged. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_library_loader.cc:49: static jboolean InitializeBlimp(JNIEnv* env, jclass clazz, jobject jcontext) { On 2015/08/28 01:23:45, David Trainor wrote: > On 2015/08/27 02:01:50, Wez wrote: > > Let's not have InitBlimp _and_ InitializeBlimp here! > > > > Where is this function even called from...? Doesn't seem to be declared in the > > .h and it's not called from this code object..? > > This is called from java (notice the JNIEnv*, and jclass, and jobject). The > header file is auto-generated from the related java file and included here (see > jni/BlimpLibraryLoader_jni.h). > > - If you see a "nativeXYZ()" in a Java file, it's backed by a native method. > - If you see a native method with parameters "JNIEnv*, jclass, ..." it's a > backing of a static Java method. > - If you see a native method with parameters "JNIEnv*, jobject, ..." it's a > backing of a non-static Java method. Acknowledged. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_library_loader.cc:83: !base::android::OnJNIOnLoadInit(init_callbacks)) { On 2015/08/28 01:23:45, David Trainor wrote: > On 2015/08/27 02:01:50, Wez wrote: > > This OnJNIOnLoadInit() stuff is strangely obfuscating in the case where you > only > > have a single initialization function. We could simply call it with no init > > callback, and continue our initialization if it returns true, and the code > would > > be simpler, surely? > > I'd rather not bypass the basic work flow (although I agree it ends up being a > bit convoluted for this case)... if they ever refactor it I don't want this > target to just be dropped on the floor. Other components (content/, chrome/, > etc.) use this. We should do the same :). Acknowledged, but perhaps include a comment to clarify that this is currently over-engineered, following the established Clank pattern, to be future-proof? https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... File blimp/client/android/blimp_view.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_view.cc:14: static jlong Init(JNIEnv* env, On 2015/08/28 01:23:45, David Trainor wrote: > On 2015/08/27 02:01:50, Wez wrote: > > Where is this called from? Can we give it a more helpful name? > > From Java. I'd like to keep the same name. It's standard across the Chromium > codebase (do a search for " Init(JNI"). > > This is backed by BlimpView#nativeInit(). The header declaration is in the > auto-generated jni/BlimpView_jni.h. Acknowledged. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_view.cc:44: compositor_.reset(); On 2015/08/28 01:23:45, David Trainor wrote: > On 2015/08/27 02:01:50, Wez wrote: > > Why do you need to reset explicitly here, rather than letting implicit dtor > take > > care of it? > > Ah I ended up refactoring some things so this might not matter anymore. There > were some issues with the other Android compositor instances across our codebase > where destruction order matters a lot. Will remove. Acknowledged. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_view.cc:68: current_surface_format_ = 0; On 2015/08/28 01:23:45, David Trainor wrote: > On 2015/08/27 02:01:50, Wez wrote: > > nit: What does surface format of zero mean? > > In the Android SDK Java file it's defined as UNKNOWN. It's not explicitly > defined in the native counterpart enum that the Android Java enum mirrors. Acknowledged. Thanks for adding the clarifying comment! https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... File blimp/client/android/blimp_view.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_view.h:33: // JNI Methods On 2015/08/28 01:23:45, David Trainor wrote: > On 2015/08/27 02:01:51, Wez wrote: > > nit: Suggest "Handlers for calls from Java." or similar, so it's clear that > > these are Java->C++, not wrappers for C++->Java? > > Updated to "Methods called from Java via JNI" > > Generally C++ -> Java isn't exposed in any non-auto generated header. Acknowledged. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_view.h:34: void Destroy(JNIEnv* env, jobject jobj); On 2015/08/28 01:23:45, David Trainor wrote: > On 2015/08/27 02:01:50, Wez wrote: > > nit: OnDestroy? > > I don't know if I agree. This actually destroys the object because it's owned > by the Java component. Destroy() feels like a call that actually does the > destruction. OnDestroy() feels like a notification call. SGTM https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_view.h:49: base::android::ScopedJavaGlobalRef<jobject> java_obj_; On 2015/08/28 01:23:45, David Trainor wrote: > On 2015/08/27 02:01:50, Wez wrote: > > nit: Document this and |current_surface_format_| > > Done. You'll see java_obj_ everywhere there's a native <-> JNI class coupling. Acknowledged. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_view.h:52: int current_surface_format_; On 2015/08/28 01:23:45, David Trainor wrote: > On 2015/08/27 02:01:50, Wez wrote: > > nit: You can initialize this in-line. > > Oh I forgot about that! Is it required? IIRC the chromium-dev@ thread seemed > to leave it open-ended. I'd rather initialize all variables together unless the > new standard is to in-line initializations. No, it's not required; it's mainly helpful if you would otherwise be able to get away w/out any member initialization in the ctor, or if you have several ctors and lots of members - although ctor delegation also helps there. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... File blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java:17: * A simple utility class to asynchronously load and register the native libraries associated with On 2015/08/28 01:23:45, David Trainor wrote: > On 2015/08/27 02:01:51, Wez wrote: > > Given how awful this looks in codereview, and that style guide allows either > 80 > > or 100 columns for Java, how would you feel about keeping Blimp .java to 80 > > columns? > > Chromium java uses the Android style guide which explicitly states 100 > characters (http://source.android.com/source/code-style.html#limit-line-length). > I'd also rather stay consistent with the rest of Chromium. It would be > somewhat awkward to expect other Chromium devs to know about/handle different > line lengths for this directory vs. all others. > > Also updated the comment based on your other class comment suggestions :). Three things: 1. Acknowledged. :P 2. *facepalm* 3. Technically a line length limit of 80 chars meets the Android style-guide requirement of at-most-100 chars. ;) https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java:62: if (callback != null) callback.onStartupComplete(startResult); On 2015/08/28 01:23:45, David Trainor wrote: > On 2015/08/27 02:01:51, Wez wrote: > > nit: This if() needs braces according to style-guide. > > Chromium Java uses the Android styleguide. > http://source.android.com/source/code-style.html#use-standard-brace-style. Most > of our Java code doesn't have {} for single line if's. Acknowledged. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... File blimp/client/android/java/src/org/chromium/blimp/BlimpView.java (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:24: private long mNativeBlimpViewPtr; On 2015/08/28 01:23:46, David Trainor wrote: > On 2015/08/27 02:01:51, Wez wrote: > > n00b: Is mFoo the preferred form for member variable names? Can't see any > > mention in style guide, either way? > > Ah yeah that's part of the Android style guide > (http://source.android.com/source/code-style.html#follow-field-naming-conventions). Acknowledged. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:36: protected void onFinishInflate() { On 2015/08/28 01:23:46, David Trainor wrote: > On 2015/08/27 02:01:51, Wez wrote: > > Add a comment to clarify which interface this is overriding? Move the > > declaration down to the relevant section below? > > It's overriding a base class method. I can move it down and add a section > though. Acknowledged. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:82: if (mNativeBlimpViewPtr == 0) return; On 2015/08/28 01:23:46, David Trainor wrote: > On 2015/08/27 02:01:51, Wez wrote: > > nit: Braces, here and below > > See java style guide comment above. Acknowledged. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:95: // SurfaceHolder.Callback2 Implementation ------------------------------------------------------ On 2015/08/28 01:23:45, David Trainor wrote: > On 2015/08/27 02:01:51, Wez wrote: > > n00b nit: I thought this trailing --- thing was frowned upon, though I can't > > find anything prohibiting it... but... do we haz to? > > I think it helps see a clear break in between sections, but I'm happy to remove > it if it's frowned upon. Die ---! My personal preference is to do that with a larger block comment e.g. // // Lots Of Stuff. // but it seems there's no guidelines in style-guide! I do have vague recollections of the lots-of-dashes style being frowned upon but perhaps that's just wishful thinking on my part. ;) https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... File blimp/client/compositor/blimp_compositor.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.cc:33: namespace { On 2015/08/28 01:23:46, David Trainor wrote: > On 2015/08/27 02:01:51, Wez wrote: > > Why does this need a separate anonymous namespace? > > I wanted it separate so I could delete this chunk once we actually draw > something in the compositor. This was just for testing purposes so something > actually displays (makes sure the scheduler/threads/pipelines are working > properly). > > I'll move it into the other namespace though :). OK, suggest commenting like: TODO(dtrainor): Replace this with a real layer driver impl (see crbug.com/...). :) https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.cc:44: // Make sure things get freed in the right order. On 2015/08/28 01:23:46, David Trainor wrote: > On 2015/08/27 02:01:51, Wez wrote: > > nit: This comment explains why we're doing it explicitly, but not the specific > > constraints. > > Done. Acknowledged. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.cc:146: false)); On 2015/08/28 01:23:46, David Trainor wrote: > On 2015/08/27 02:01:51, Wez wrote: > > What's all this about? Surely you can just create the thread with a normal > > messgae-loop and IO won't be allowed on it? > > I pulled this piece from render_thread_impl.cc. Looking at > thread_restrictions.cc, it looks like the lazy ThreadLocalBoolean is called > g_io_disallowed (which defaults to false, which means IO is allowed). Yay > double negatives? I think I have to do this to turn off IO on this thread. Ha! We have a poster about avoiding negatives in conditionals up on the wall nearby right now. :P Anyway... Ewwww, gross, but seemingly necessary. Do we also want to disallow waiting? Looks like Chromoting does that on its network thread, but I don't see Chrome doing that. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... File blimp/client/compositor/blimp_compositor.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.h:42: // LayerTreeHostClient Implementation ---------------------------------------- On 2015/08/28 01:23:46, David Trainor wrote: > On 2015/08/27 02:01:51, Wez wrote: > > n00b nit: See my comment in the .java re trailing ---- > > > > nit: Elsewhere in Chromium we have comments like: > > > > // WibbleFlanger interface. > > > > rather than "implementation" - since this isn't actually the implementation. > > Be gone ---! I was looking at the cc files and they seem to do "<ClassName> > implementation", which makes sense to me. This is the implementation though, > right? We're implementating the interface here? Sadly the C++ is not very consistent; typically in C++ this comment is in the header, which is where you are declaring the bits of the interface that you're overriding, so I prefer "Foo interface." so the comment indicates which interface the stuff you're overriding belongs to. In this case there's no separation between declaration and definition, so "Foo implementation." would be reasonable - I'd personally still argue that "Foo interface." is still valid, though, since it explains which interface the overrides implement, and is then consistent with what I'd expect in C++. This is super nit-picky, though! So long as we're consistent, it's all good. :) p.s. You're missing . here too, at the end of the comment. :P https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.h:43: void WillBeginMainFrame() override {} On 2015/08/28 01:23:46, David Trainor wrote: > On 2015/08/27 02:01:51, Wez wrote: > > Chromium style guide requires that virtual functions _never_ be defined > in-line. > > Ah my bad! I saw this in ui/compositor/compositor.h and > compositor_impl_android.h when I was looking for the right way to implement > this. Are these all incorrect? I can look into fixing those in another patch > if so. Strictly speaking, yes; I'm pretty sure the guidance used to be in favour of inline {} for empty overrides, which is probably why those classes do it. I think it later transpired that inline overrides impacts link times or something, hence the Chromium style-guide dictat. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.h:82: virtual void GenerateLayerTreeSettings(cc::LayerTreeSettings& settings, On 2015/08/28 01:23:46, David Trainor wrote: > On 2015/08/27 02:01:51, Wez wrote: > > If this is an out-parameter then it should be cc::LayerTreeSettings*, not &. > > > > I'm unclear on what cc::LayerTreeSettings intended usage is; it is declared as > a > > class but in all respects looks like a struct. All of it's members are PoD or > > PoD-equivalent AFAICT, so can you just have this be: > > > > virtual cc::LayerTreeSettings GenerateLayerTreeSettings(const > base::CommandLine& > > cmd); > > I changed it to LayerTreeSettings*. Acknowledged. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... File blimp/client/compositor/blimp_compositor_android.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.h:36: void SetSurface(JNIEnv* env, jobject jsurface); On 2015/08/28 01:23:47, David Trainor wrote: > On 2015/08/27 02:01:52, Wez wrote: > > nit: Comment to explain e.g. ownership semantics for jsurface. > > > > Incidentally, why jsurface, not just surface? > > Done. > > I do jsurface because it's a Java object. This makes it easy when marshalling > to native objects because the native version can have the same name - the j. Acknowledged. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.h:53: gfx::AcceleratedWidget window_; On 2015/08/28 01:23:47, David Trainor wrote: > On 2015/08/27 02:01:52, Wez wrote: > > nit: Initialize in-line? > > Still would prefer all constructor initialization, unless it's required :(. Acknowledged. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... File blimp/client/android/blimp_view.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... blimp/client/android/blimp_view.h:28: // system decorations (See android.view.Display#getRealSize()). nit: You mean that this is the total display area, including the area occupied by system controls, whereas getSize() is the display area not occupied by system controls? https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... blimp/client/android/blimp_view.h:32: // (device independent pixels) to pixels. nit: Aren't Device Independent Pixels called DIPs? https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... blimp/client/android/blimp_view.h:39: // Methods called from Java via JNI nit: Missing . https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... blimp/client/android/blimp_view.h:55: // A reference to the Java object which owns this class. nit: No need for "A" here. So this reference has no effect on GC...? i.e. we're not creating a reference-loop here? https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... File blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java:68: // Native Methods nit: Missing . ,and capitalization on "methods"? https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... File blimp/client/android/java/src/org/chromium/blimp/BlimpRendererActivity.java (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... blimp/client/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:43: // BlimpLibraryLoader.Callback Implementation nit: implementation and . https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... File blimp/client/android/java/src/org/chromium/blimp/BlimpView.java (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:87: // SurfaceView overrides nit: . https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:96: // SurfaceHolder.Callback2 implementation nit: implementation -> interface? https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... File blimp/client/compositor/blimp_compositor.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_compositor.h:88: // A lazily created thread that will run the compositor rendering tasks. nit: No need for "A", I don't think. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... File blimp/client/compositor/blimp_compositor_android.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.cc:18: // The minimum size increase for tiles (we want them to be multiples of 32). nit: "size increase" doesn't mean much in this context; might be clearer to express this as the block size of which tile sizes must be a multiple? Is it the case that the minimum content width must be a multiple of this value? https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.cc:92: int right_tile_width = portrait_width_ % default_tile_size; Isn't the rightmost tile width: ((portrait_width_-1) % default_tile_size)+1 i.e. ranging from 1->default_tile_size, not 0->default_tile_size-1? That would let you get rid of the zero special-case below. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.cc:101: static_cast<float>(num_full_tiles * kTileSizeIncrementStepPixels)); It's not clear to me what the connection between the comment and the algebra is here; it looks like you're trying to work out a small amount to expand the default tile size by to avoid having a thin sliver down the RHS - so basically you want to split the right-tile-width amongst the full tiles? kTileSizeIncrementStepPixels only factors in because you require tiles to be a multiple of that size - so you should just be able to calculate the ideal target size and round-up, surely? https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... File blimp/client/compositor/blimp_compositor_android.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.h:44: // to SetSurface or when this class dies. nit: This comment should be of the form: Takes a reference to the ANativeWindow backing |jsurface|, to use to render to, and releases any previously-held reference. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.h:52: // from dp (device independent pixels) to pixels. nit: to device pixels. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.h:63: // These are used to determine tile size for the compositor's rastered tiles. nit: Drop "These are" https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.h:70: // size of the device (includes the system decorations) or not. nit: Suggest "True if the |portrait_width_| and |landscape_width_| represent the device's physical dimensions, including any area occupied by system decorations.", or just copy the comment for this parameter from the ctor comment, above? Should this just be called "is_full_screen_"? https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... File blimp/client/compositor/blimp_context_provider.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_context_provider.cc:168: // There's no memory manager for the in-process implementation. Does this mean this should never be called, in which case NOTREACHED()? Or that it is not-yet-implemented, in which case NOTIMPLEMENTED()? https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... File blimp/client/compositor/blimp_context_provider.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_context_provider.h:25: // cc::ContextProvider implementation nit: Missing . https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... File blimp/client/compositor/blimp_output_surface.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_output_surface.h:15: // A minimal implementation of cc::OutputSurface. nit: No need for "A" https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_output_surface.h:22: // OutputSurface implementation nit: Missing . https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... File blimp/client/compositor/blimp_task_graph_runner.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_task_graph_runner.h:14: // A TaskGraphRunner that runs on a single thread. See cc::TaskGraphRunner for nit: No need for "A" https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_task_graph_runner.h:23: // base::DelegateSimpleThread::Delegate implementation nit: Missing . https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/BU... File blimp/common/BUILD.gn (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/BU... blimp/common/BUILD.gn:8: # are finalized or are pushed from the server component. nit: Do we have a bug filed to track that work, that you can refer to here? https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/DEPS File blimp/common/DEPS (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/DE... blimp/common/DEPS:11: # are finalized or are pushed from the server component. Bug # for that work? https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/co... File blimp/common/compositor/blimp_layer_tree_settings.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/co... blimp/common/compositor/blimp_layer_tree_settings.h:24: // Contains the basic compositor settings that are shared between the common Suggest: "Populates |settings| based on defaults & flags in |cmd|. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/co... blimp/common/compositor/blimp_layer_tree_settings.h:25: // blimp compositors instances. This might have to be tweaked into a message nit: Blimp compositor instances. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/co... blimp/common/compositor/blimp_layer_tree_settings.h:26: // protocol if it turns one one compositor needs to actually drive some specific nit: out one https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/co... blimp/common/compositor/blimp_layer_tree_settings.h:27: // settings of the other. This comment's a bit opaque - what are these different instances for (different tabs...?) and why would they need to drive one another? https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/co... blimp/common/compositor/blimp_layer_tree_settings.h:30: const base::CommandLine& cmd); nit: command_line Does nothing in Android/C++ init set the process-global base::CommandLine so we can avoid needing to pass it explicitly here?
https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/blimp.gyp File blimp/blimp.gyp (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/blimp.gyp#newc... blimp/blimp.gyp:76: 'apk_name': 'Blimp', On 2015/08/27 02:01:50, Wez wrote: > nit: Do we want the APK name capitalized in this way? Is there a convention > requiring it, for example? Ah sorry I missed this comment earlier. I took a look through Chromium and all apk names seem to start with capital letters for each word (ChromeShell, ChromePublic, Chrome, etc.). https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/DEPS File blimp/client/DEPS (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/DEPS#ne... blimp/client/DEPS:4: "+content/public/common", On 2015/08/27 02:01:50, Wez wrote: > On 2015/08/19 07:48:06, nyquist (OOO - back 8-24) wrote: > > Could we have > > "-content", > > "!content/public/common/content_switches.h" instead? I think I would find it > > helpful if we instead clearly disallowed content with -content and then the > > temporary rule for the switches until we remove that dependency. > > > > Or do you think we will end up needing a lot of //content? > > Feels cleanest to disallow content & then allow specific bits that we have ugly > deps on - my 2c. Yeah agreed :) https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... File blimp/client/android/blimp_jni_registrar.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/blimp_jni_registrar.cc:13: static base::android::RegistrationMethod kBlimpRegistrationMethods[] = { On 2015/08/27 02:01:50, Wez wrote: > const? > > Why does this need to be static if it's already in an anon namespace? > > Could the constants be moved into RegisterBlimpJni, since they're only used > there? Ah good point about the static. It's generally specified outside the method, or at least that's how the other call sites do it. https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... File blimp/client/android/blimp_jni_registrar.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/blimp_jni_registrar.h:12: bool RegisterBlimpJni(JNIEnv* env); On 2015/08/27 02:01:50, Wez wrote: > nit: Comment to explain what this does, who owns |env| & why it's req'd, what > the return value means? e.g. does a return value of false mean All Is Lost...? I'll try to read up on it and add a comment (the same general structure used in about 50 places for all of Chromium's native/Java bridge setups. I've always known I *have* to do it, but I don't know the inner workings of all of these components :/). https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... File blimp/client/android/blimp_library_loader.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/blimp_library_loader.cc:25: bool LibraryLoaded(JNIEnv* env, jclass clazz) { On 2015/08/27 02:01:50, Wez wrote: > nit: OnLibraryLoaded Done. https://chromiumcodereview.appspot.com/1295243003/diff/1/blimp/client/android... blimp/client/android/blimp_library_loader.cc:64: base::MessageLoopForUI::current()->Start(); On 2015/08/27 02:01:50, Wez wrote: > So.. this is some fun hack around us not actually having control of the process' > main thread? This is because the message loop is actually driven by Java, so we need to call Start() and not actually "Run" the message loop. Java needs to own it because the entire Android Java framework is also based on message loops/runnables/etc.. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... File blimp/client/android/blimp_library_loader.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_library_loader.cc:83: !base::android::OnJNIOnLoadInit(init_callbacks)) { On 2015/09/03 00:49:26, Wez wrote: > On 2015/08/28 01:23:45, David Trainor wrote: > > On 2015/08/27 02:01:50, Wez wrote: > > > This OnJNIOnLoadInit() stuff is strangely obfuscating in the case where you > > only > > > have a single initialization function. We could simply call it with no init > > > callback, and continue our initialization if it returns true, and the code > > would > > > be simpler, surely? > > > > I'd rather not bypass the basic work flow (although I agree it ends up being a > > bit convoluted for this case)... if they ever refactor it I don't want this > > target to just be dropped on the floor. Other components (content/, chrome/, > > etc.) use this. We should do the same :). > > Acknowledged, but perhaps include a comment to clarify that this is currently > over-engineered, following the established Clank pattern, to be future-proof? Done. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... File blimp/client/android/blimp_view.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/blimp_view.h:52: int current_surface_format_; On 2015/09/03 00:49:26, Wez wrote: > On 2015/08/28 01:23:45, David Trainor wrote: > > On 2015/08/27 02:01:50, Wez wrote: > > > nit: You can initialize this in-line. > > > > Oh I forgot about that! Is it required? IIRC the chromium-dev@ thread seemed > > to leave it open-ended. I'd rather initialize all variables together unless > the > > new standard is to in-line initializations. > > No, it's not required; it's mainly helpful if you would otherwise be able to get > away w/out any member initialization in the ctor, or if you have several ctors > and lots of members - although ctor delegation also helps there. Good point. I'll move towards this in the future if I can get rid of all member initialization in the constructors :). https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... File blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java:17: * A simple utility class to asynchronously load and register the native libraries associated with On 2015/09/03 00:49:26, Wez wrote: > On 2015/08/28 01:23:45, David Trainor wrote: > > On 2015/08/27 02:01:51, Wez wrote: > > > Given how awful this looks in codereview, and that style guide allows either > > 80 > > > or 100 columns for Java, how would you feel about keeping Blimp .java to 80 > > > columns? > > > > Chromium java uses the Android style guide which explicitly states 100 > > characters > (http://source.android.com/source/code-style.html#limit-line-length). > > I'd also rather stay consistent with the rest of Chromium. It would be > > somewhat awkward to expect other Chromium devs to know about/handle different > > line lengths for this directory vs. all others. > > > > Also updated the comment based on your other class comment suggestions :). > > Three things: > 1. Acknowledged. :P > 2. *facepalm* > 3. Technically a line length limit of 80 chars meets the Android style-guide > requirement of at-most-100 chars. ;) Re 3: Made me laugh out loud! :) https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... File blimp/client/android/java/src/org/chromium/blimp/BlimpView.java (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/an... blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:95: // SurfaceHolder.Callback2 Implementation ------------------------------------------------------ On 2015/09/03 00:49:27, Wez wrote: > On 2015/08/28 01:23:45, David Trainor wrote: > > On 2015/08/27 02:01:51, Wez wrote: > > > n00b nit: I thought this trailing --- thing was frowned upon, though I can't > > > find anything prohibiting it... but... do we haz to? > > > > I think it helps see a clear break in between sections, but I'm happy to > remove > > it if it's frowned upon. Die ---! > > My personal preference is to do that with a larger block comment e.g. > > // > // Lots Of Stuff. > // > > but it seems there's no guidelines in style-guide! I do have vague recollections > of the lots-of-dashes style being frowned upon but perhaps that's just wishful > thinking on my part. ;) I removed them all. I think you're right and this looks fine :). https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... File blimp/client/compositor/blimp_compositor.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.cc:33: namespace { On 2015/09/03 00:49:27, Wez wrote: > On 2015/08/28 01:23:46, David Trainor wrote: > > On 2015/08/27 02:01:51, Wez wrote: > > > Why does this need a separate anonymous namespace? > > > > I wanted it separate so I could delete this chunk once we actually draw > > something in the compositor. This was just for testing purposes so something > > actually displays (makes sure the scheduler/threads/pipelines are working > > properly). > > > > I'll move it into the other namespace though :). > > OK, suggest commenting like: > > TODO(dtrainor): Replace this with a real layer driver impl (see crbug.com/...). > > :) Done. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.cc:146: false)); On 2015/09/03 00:49:27, Wez wrote: > On 2015/08/28 01:23:46, David Trainor wrote: > > On 2015/08/27 02:01:51, Wez wrote: > > > What's all this about? Surely you can just create the thread with a normal > > > messgae-loop and IO won't be allowed on it? > > > > I pulled this piece from render_thread_impl.cc. Looking at > > thread_restrictions.cc, it looks like the lazy ThreadLocalBoolean is called > > g_io_disallowed (which defaults to false, which means IO is allowed). Yay > > double negatives? I think I have to do this to turn off IO on this thread. > > Ha! We have a poster about avoiding negatives in conditionals up on the wall > nearby right now. :P > > Anyway... Ewwww, gross, but seemingly necessary. Do we also want to disallow > waiting? Looks like Chromoting does that on its network thread, but I don't see > Chrome doing that. Yes! I want a poster like that. I had to ask Tommy if I was reading the logic correctly. I'm not sure about waiting. I'll add a TODO and investigate when I get back from vacation if that's okay! https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... File blimp/client/compositor/blimp_compositor.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.h:42: // LayerTreeHostClient Implementation ---------------------------------------- On 2015/09/03 00:49:27, Wez wrote: > On 2015/08/28 01:23:46, David Trainor wrote: > > On 2015/08/27 02:01:51, Wez wrote: > > > n00b nit: See my comment in the .java re trailing ---- > > > > > > nit: Elsewhere in Chromium we have comments like: > > > > > > // WibbleFlanger interface. > > > > > > rather than "implementation" - since this isn't actually the implementation. > > > > Be gone ---! I was looking at the cc files and they seem to do "<ClassName> > > implementation", which makes sense to me. This is the implementation though, > > right? We're implementating the interface here? > > Sadly the C++ is not very consistent; typically in C++ this comment is in the > header, which is where you are declaring the bits of the interface that you're > overriding, so I prefer "Foo interface." so the comment indicates which > interface the stuff you're overriding belongs to. > > In this case there's no separation between declaration and definition, so "Foo > implementation." would be reasonable - I'd personally still argue that "Foo > interface." is still valid, though, since it explains which interface the > overrides implement, and is then consistent with what I'd expect in C++. > > This is super nit-picky, though! So long as we're consistent, it's all good. :) > > p.s. You're missing . here too, at the end of the comment. :P Crap! Adding . to all "X implementation" lines. https://chromiumcodereview.appspot.com/1295243003/diff/100001/blimp/client/co... blimp/client/compositor/blimp_compositor.h:43: void WillBeginMainFrame() override {} On 2015/09/03 00:49:27, Wez wrote: > On 2015/08/28 01:23:46, David Trainor wrote: > > On 2015/08/27 02:01:51, Wez wrote: > > > Chromium style guide requires that virtual functions _never_ be defined > > in-line. > > > > Ah my bad! I saw this in ui/compositor/compositor.h and > > compositor_impl_android.h when I was looking for the right way to implement > > this. Are these all incorrect? I can look into fixing those in another patch > > if so. > > Strictly speaking, yes; I'm pretty sure the guidance used to be in favour of > inline {} for empty overrides, which is probably why those classes do it. I > think it later transpired that inline overrides impacts link times or something, > hence the Chromium style-guide dictat. Moved these all to the cc file. I'm fine with that :). https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... File blimp/client/android/blimp_view.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... blimp/client/android/blimp_view.h:28: // system decorations (See android.view.Display#getRealSize()). On 2015/09/03 00:49:27, Wez wrote: > nit: You mean that this is the total display area, including the area occupied > by system controls, whereas getSize() is the display area not occupied by system > controls? I think Android calls it system decoration, but I like the rest of your wording much better. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... blimp/client/android/blimp_view.h:32: // (device independent pixels) to pixels. On 2015/09/03 00:49:27, Wez wrote: > nit: Aren't Device Independent Pixels called DIPs? Yeah but Android changed the unit of measurement from dip to dp a few SDK revisions ago IIRC. They both still work, but most of the more recent Android code you'll see will have "10dp." Not sure why they moved over... I think it's to mimic "sp" for font sizes. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... blimp/client/android/blimp_view.h:39: // Methods called from Java via JNI On 2015/09/03 00:49:27, Wez wrote: > nit: Missing . Done. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... blimp/client/android/blimp_view.h:55: // A reference to the Java object which owns this class. On 2015/09/03 00:49:27, Wez wrote: > nit: No need for "A" here. > > So this reference has no effect on GC...? i.e. we're not creating a > reference-loop here? We are. That's on purpose though. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... File blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java:68: // Native Methods On 2015/09/03 00:49:27, Wez wrote: > nit: Missing . ,and capitalization on "methods"? Done. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... File blimp/client/android/java/src/org/chromium/blimp/BlimpRendererActivity.java (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... blimp/client/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:43: // BlimpLibraryLoader.Callback Implementation On 2015/09/03 00:49:27, Wez wrote: > nit: implementation and . Done. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... File blimp/client/android/java/src/org/chromium/blimp/BlimpView.java (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:87: // SurfaceView overrides On 2015/09/03 00:49:27, Wez wrote: > nit: . Done. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:96: // SurfaceHolder.Callback2 implementation On 2015/09/03 00:49:27, Wez wrote: > nit: implementation -> interface? Done. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... File blimp/client/compositor/blimp_compositor.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_compositor.h:88: // A lazily created thread that will run the compositor rendering tasks. On 2015/09/03 00:49:27, Wez wrote: > nit: No need for "A", I don't think. Done. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... File blimp/client/compositor/blimp_compositor_android.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.cc:18: // The minimum size increase for tiles (we want them to be multiples of 32). On 2015/09/03 00:49:28, Wez wrote: > nit: "size increase" doesn't mean much in this context; might be clearer to > express this as the block size of which tile sizes must be a multiple? > > Is it the case that the minimum content width must be a multiple of this value? I'm not sure. I don't think so. I think the tiles need to be this size because it's more efficient for the GPU, but I haven't seen any code that requires the content area to match this. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.cc:92: int right_tile_width = portrait_width_ % default_tile_size; On 2015/09/03 00:49:27, Wez wrote: > Isn't the rightmost tile width: > > ((portrait_width_-1) % default_tile_size)+1 > > i.e. ranging from 1->default_tile_size, not 0->default_tile_size-1? > > That would let you get rid of the zero special-case below. Good idea. Done. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.cc:101: static_cast<float>(num_full_tiles * kTileSizeIncrementStepPixels)); On 2015/09/03 00:49:28, Wez wrote: > It's not clear to me what the connection between the comment and the algebra is > here; it looks like you're trying to work out a small amount to expand the > default tile size by to avoid having a thin sliver down the RHS - so basically > you want to split the right-tile-width amongst the full tiles? > kTileSizeIncrementStepPixels only factors in because you require tiles to be a > multiple of that size - so you should just be able to calculate the ideal target > size and round-up, surely? Done. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... File blimp/client/compositor/blimp_compositor_android.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.h:44: // to SetSurface or when this class dies. On 2015/09/03 00:49:28, Wez wrote: > nit: This comment should be of the form: > > Takes a reference to the ANativeWindow backing |jsurface|, to use to render to, > and releases any previously-held reference. Done. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.h:52: // from dp (device independent pixels) to pixels. On 2015/09/03 00:49:28, Wez wrote: > nit: to device pixels. Done. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.h:63: // These are used to determine tile size for the compositor's rastered tiles. On 2015/09/03 00:49:28, Wez wrote: > nit: Drop "These are" Done. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.h:70: // size of the device (includes the system decorations) or not. On 2015/09/03 00:49:28, Wez wrote: > nit: Suggest "True if the |portrait_width_| and |landscape_width_| represent the > device's physical dimensions, including any area occupied by system > decorations.", or just copy the comment for this parameter from the ctor > comment, above? > > Should this just be called "is_full_screen_"? Done. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... File blimp/client/compositor/blimp_context_provider.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_context_provider.cc:168: // There's no memory manager for the in-process implementation. On 2015/09/03 00:49:28, Wez wrote: > Does this mean this should never be called, in which case NOTREACHED()? Or that > it is not-yet-implemented, in which case NOTIMPLEMENTED()? Oddly I think neither! I think it's more that we'll just never call them back because we don't have a memory manager, so we don't have to save the callback. I don't know if we'll need one later, but I don't think we need one for v0.5. I'll read up on this more and add a TODO though. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... File blimp/client/compositor/blimp_context_provider.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_context_provider.h:25: // cc::ContextProvider implementation On 2015/09/03 00:49:28, Wez wrote: > nit: Missing . Done. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... File blimp/client/compositor/blimp_output_surface.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_output_surface.h:15: // A minimal implementation of cc::OutputSurface. On 2015/09/03 00:49:28, Wez wrote: > nit: No need for "A" Done. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_output_surface.h:22: // OutputSurface implementation On 2015/09/03 00:49:28, Wez wrote: > nit: Missing . Done. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... File blimp/client/compositor/blimp_task_graph_runner.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_task_graph_runner.h:14: // A TaskGraphRunner that runs on a single thread. See cc::TaskGraphRunner for On 2015/09/03 00:49:28, Wez wrote: > nit: No need for "A" Done. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/co... blimp/client/compositor/blimp_task_graph_runner.h:23: // base::DelegateSimpleThread::Delegate implementation On 2015/09/03 00:49:28, Wez wrote: > nit: Missing . Done. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/BU... File blimp/common/BUILD.gn (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/BU... blimp/common/BUILD.gn:8: # are finalized or are pushed from the server component. On 2015/09/03 00:49:28, Wez wrote: > nit: Do we have a bug filed to track that work, that you can refer to here? We don't, but I'll file one. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/DEPS File blimp/common/DEPS (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/DE... blimp/common/DEPS:11: # are finalized or are pushed from the server component. On 2015/09/03 00:49:28, Wez wrote: > Bug # for that work? Done. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/co... File blimp/common/compositor/blimp_layer_tree_settings.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/co... blimp/common/compositor/blimp_layer_tree_settings.h:24: // Contains the basic compositor settings that are shared between the common On 2015/09/03 00:49:28, Wez wrote: > Suggest: "Populates |settings| based on defaults & flags in |cmd|. Done. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/co... blimp/common/compositor/blimp_layer_tree_settings.h:25: // blimp compositors instances. This might have to be tweaked into a message On 2015/09/03 00:49:28, Wez wrote: > nit: Blimp compositor instances. Done. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/co... blimp/common/compositor/blimp_layer_tree_settings.h:26: // protocol if it turns one one compositor needs to actually drive some specific On 2015/09/03 00:49:28, Wez wrote: > nit: out one Done. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/co... blimp/common/compositor/blimp_layer_tree_settings.h:27: // settings of the other. On 2015/09/03 00:49:28, Wez wrote: > This comment's a bit opaque - what are these different instances for (different > tabs...?) and why would they need to drive one another? Fixed. I meant the host driving the settings of the client. https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/co... blimp/common/compositor/blimp_layer_tree_settings.h:30: const base::CommandLine& cmd); On 2015/09/03 00:49:28, Wez wrote: > nit: command_line > > Does nothing in Android/C++ init set the process-global base::CommandLine so we > can avoid needing to pass it explicitly here? It does :). Done.
https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... File blimp/client/android/blimp_view.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... blimp/client/android/blimp_view.h:32: // (device independent pixels) to pixels. On 2015/09/03 06:33:21, David Trainor (OOO) wrote: > On 2015/09/03 00:49:27, Wez wrote: > > nit: Aren't Device Independent Pixels called DIPs? > > Yeah but Android changed the unit of measurement from dip to dp a few SDK > revisions ago IIRC. They both still work, but most of the more recent Android > code you'll see will have "10dp." Not sure why they moved over... I think it's > to mimic "sp" for font sizes. According to http://developer.android.com/guide/topics/resources/more-resources.html#Dimen... "dp" is short-hand for "Density-independent Pixel" and "px" is short-hand for "actual pixel" (i.e. "device" or "physical" pixels). So I'd recommend renaming dp_to_pixel to dp_to_px, and updating the decription to use that page's terminology. You'll also want to make explicit which units the physical & display sizes are in. Finally, you could include the reference to the Dimensions doc here, if you think it'd be a useful reference to avoid having to describe all this to n00bs like me. ;) https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... blimp/client/android/blimp_view.h:55: // A reference to the Java object which owns this class. On 2015/09/03 06:33:21, David Trainor (OOO) wrote: > On 2015/09/03 00:49:27, Wez wrote: > > nit: No need for "A" here. > > > > So this reference has no effect on GC...? i.e. we're not creating a > > reference-loop here? > > We are. That's on purpose though. OK - sounds like the ownership graph needs clarifying, then? https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/BU... File blimp/common/BUILD.gn (right): https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/BU... blimp/common/BUILD.gn:8: # are finalized or are pushed from the server component. On 2015/09/03 06:33:22, David Trainor (OOO) wrote: > On 2015/09/03 00:49:28, Wez wrote: > > nit: Do we have a bug filed to track that work, that you can refer to here? > > We don't, but I'll file one. Thanks. :) https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... File blimp/client/android/blimp_jni_registrar.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... blimp/client/android/blimp_jni_registrar.h:15: // the native Java VM code. I'd suggest: "Registers native method hooks with the Java runtime specified by |env|. Returns false if registration fails, in which case native methods are unavailable to Java." It wasn't clear to me reading the code whether the hooks actually retain references to |env| or whether this method is self-contained? https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... File blimp/client/android/blimp_library_loader.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... blimp/client/android/blimp_library_loader.cc:97: // calling these methods makes us future-proof for underlying JNI refactors. nit: I think: "Although we only need to register JNI for base/ and blimp/, this follows the general Chrome for Android pattern, to be future-proof against future changes to JNI." https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... File blimp/client/android/blimp_view.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... blimp/client/android/blimp_view.h:31: // (device pixels) to pixels. Confused - I thought dp was Device/Density Independent Pixels, i.e. logical pixels, and "pixel" was actual physical pixels? https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... blimp/client/android/blimp_view.h:35: const gfx::Size& display_size, Why not name these |real_size| and |size|, for consistency with android.view.Display, OR name them |full_display_size| and |display_size| if you want to be a little more descriptive, say? "physical" size is confusing since it implies the distinction is between dp and px, for example! https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... File blimp/client/compositor/blimp_compositor_android.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.cc:86: // tile when in portrait mode. This helps worst case scroll/raster Why do we only do this optimization for real_size_supported_? https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.cc:88: int right_tile_width = (portrait_width_ - 1) % default_tile_size + 1; nit: Brackets around the % to make it clear that it's (A % B) +1, not A & (B + 1) https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.cc:93: 1; Isn't ceil(A/B) - 1 in practice floor(A/B) in this situation? i.e. we can just say: full_tile_count = A / B; and then say that we're going to expand the tile size to ensure we have only that many tiles? https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.cc:100: // Round up to nearest 32. nit: Explain why - sounds like this is for GPU efficiency? https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... File blimp/client/compositor/blimp_compositor_android.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.h:33: // (device pixels) to pixels. See comment on the BlimpView and note that you've called dp_to_pixel "device_scale_factor" here, which is confusing! https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.h:50: // (device pixels) to pixels. This is a copy of the comment on Create() - it doesn't describe any of the actual parameters of this ctor, at least not using their actual names!
On Thu, Sep 3, 2015 at 11:26 AM, <wez@chromium.org> wrote: > > > https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... > File blimp/client/android/blimp_view.h (right): > > > https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... > blimp/client/android/blimp_view.h:32: // (device independent pixels) to > pixels. > On 2015/09/03 06:33:21, David Trainor (OOO) wrote: > >> On 2015/09/03 00:49:27, Wez wrote: >> > nit: Aren't Device Independent Pixels called DIPs? >> > > Yeah but Android changed the unit of measurement from dip to dp a few >> > SDK > >> revisions ago IIRC. They both still work, but most of the more recent >> > Android > >> code you'll see will have "10dp." Not sure why they moved over... I >> > think it's > >> to mimic "sp" for font sizes. >> > > According to > > http://developer.android.com/guide/topics/resources/more-resources.html#Dimen... > "dp" is short-hand for "Density-independent Pixel" and "px" is > short-hand for "actual pixel" (i.e. "device" or "physical" pixels). > So I'd recommend renaming dp_to_pixel to dp_to_px, and updating the > decription to use that page's terminology. You'll also want to make > explicit which units the physical & display sizes are in. > FWIW all the chromium code that I know of that talks about density/device-independent pixels calls them either something like "NNN space pixels" (nicely precise about what space it is, cc uses this) or just DIPs (the aura ui code uses this). And physical pixels are called physical pixels (in cc) or just pixels (in ui/aura). So I mildly prefer not adding a third naming scheme. Examples: https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/dip_... https://code.google.com/p/chromium/codesearch#chromium/src/cc/quads/render_pa... https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre... https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/layer_im... https://code.google.com/p/chromium/codesearch#chromium/src/cc/quads/draw_quad... > > Finally, you could include the reference to the Dimensions doc here, if > you think it'd be a useful reference to avoid having to describe all > this to n00bs like me. ;) > > > https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... > blimp/client/android/blimp_view.h:55: // A reference to the Java object > which owns this class. > On 2015/09/03 06:33:21, David Trainor (OOO) wrote: > >> On 2015/09/03 00:49:27, Wez wrote: >> > nit: No need for "A" here. >> > >> > So this reference has no effect on GC...? i.e. we're not creating a >> > reference-loop here? >> > > We are. That's on purpose though. >> > > OK - sounds like the ownership graph needs clarifying, then? > > > https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/BU... > File blimp/common/BUILD.gn (right): > > > https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/BU... > blimp/common/BUILD.gn:8: # are finalized or are pushed from the server > component. > On 2015/09/03 06:33:22, David Trainor (OOO) wrote: > >> On 2015/09/03 00:49:28, Wez wrote: >> > nit: Do we have a bug filed to track that work, that you can refer >> > to here? > > We don't, but I'll file one. >> > > Thanks. :) > > > https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... > File blimp/client/android/blimp_jni_registrar.h (right): > > > https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... > blimp/client/android/blimp_jni_registrar.h:15: // the native Java VM > code. > I'd suggest: > > "Registers native method hooks with the Java runtime specified by |env|. > Returns false if registration fails, in which case native methods are > unavailable to Java." > > It wasn't clear to me reading the code whether the hooks actually retain > references to |env| or whether this method is self-contained? > > > https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... > File blimp/client/android/blimp_library_loader.cc (right): > > > https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... > blimp/client/android/blimp_library_loader.cc:97: // calling these > methods makes us future-proof for underlying JNI refactors. > nit: I think: > > "Although we only need to register JNI for base/ and blimp/, this > follows the general Chrome for Android pattern, to be future-proof > against future changes to JNI." > > > https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... > File blimp/client/android/blimp_view.h (right): > > > https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... > blimp/client/android/blimp_view.h:31: // (device pixels) to pixels. > Confused - I thought dp was Device/Density Independent Pixels, i.e. > logical pixels, and "pixel" was actual physical pixels? > > > https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... > blimp/client/android/blimp_view.h:35: const gfx::Size& display_size, > Why not name these |real_size| and |size|, for consistency with > android.view.Display, OR name them |full_display_size| and > |display_size| if you want to be a little more descriptive, say? > "physical" size is confusing since it implies the distinction is between > dp and px, for example! > > > https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... > File blimp/client/compositor/blimp_compositor_android.cc (right): > > > https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... > blimp/client/compositor/blimp_compositor_android.cc:86: // tile when in > portrait mode. This helps worst case scroll/raster > Why do we only do this optimization for real_size_supported_? > > > https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... > blimp/client/compositor/blimp_compositor_android.cc:88: int > right_tile_width = (portrait_width_ - 1) % default_tile_size + 1; > nit: Brackets around the % to make it clear that it's (A % B) +1, not A > & (B + 1) > > > https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... > blimp/client/compositor/blimp_compositor_android.cc:93: 1; > Isn't ceil(A/B) - 1 in practice floor(A/B) in this situation? i.e. we > can just say: > > full_tile_count = A / B; > > and then say that we're going to expand the tile size to ensure we have > only that many tiles? > > > https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... > blimp/client/compositor/blimp_compositor_android.cc:100: // Round up to > nearest 32. > nit: Explain why - sounds like this is for GPU efficiency? > > > https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... > File blimp/client/compositor/blimp_compositor_android.h (right): > > > https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... > blimp/client/compositor/blimp_compositor_android.h:33: // (device > pixels) to pixels. > See comment on the BlimpView and note that you've called dp_to_pixel > "device_scale_factor" here, which is confusing! > > > https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... > blimp/client/compositor/blimp_compositor_android.h:50: // (device > pixels) to pixels. > This is a copy of the comment on Create() - it doesn't describe any of > the actual parameters of this ctor, at least not using their actual > names! > > https://chromiumcodereview.appspot.com/1295243003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Agreed; I'm happy so long as we have consistent terminology! My preference would be for "DIPS" and "physical pixels" w/ "dp" and "px" as shorthand for variable names where we need to disambiguate, such as |dp_to_px|. Or if people prefer simply "DIPs" and "pixels" then we can avoid short-hand forms for variable names. :D On 3 September 2015 at 11:31, Dana Jansens <danakj@chromium.org> wrote: > On Thu, Sep 3, 2015 at 11:26 AM, <wez@chromium.org> wrote: > >> >> >> https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... >> File blimp/client/android/blimp_view.h (right): >> >> >> https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... >> blimp/client/android/blimp_view.h:32: // (device independent pixels) to >> pixels. >> On 2015/09/03 06:33:21, David Trainor (OOO) wrote: >> >>> On 2015/09/03 00:49:27, Wez wrote: >>> > nit: Aren't Device Independent Pixels called DIPs? >>> >> >> Yeah but Android changed the unit of measurement from dip to dp a few >>> >> SDK >> >>> revisions ago IIRC. They both still work, but most of the more recent >>> >> Android >> >>> code you'll see will have "10dp." Not sure why they moved over... I >>> >> think it's >> >>> to mimic "sp" for font sizes. >>> >> >> According to >> >> http://developer.android.com/guide/topics/resources/more-resources.html#Dimen... >> "dp" is short-hand for "Density-independent Pixel" and "px" is >> short-hand for "actual pixel" (i.e. "device" or "physical" pixels). > > >> So I'd recommend renaming dp_to_pixel to dp_to_px, and updating the >> decription to use that page's terminology. You'll also want to make >> explicit which units the physical & display sizes are in. >> > > FWIW all the chromium code that I know of that talks about > density/device-independent pixels calls them either something like "NNN > space pixels" (nicely precise about what space it is, cc uses this) or just > DIPs (the aura ui code uses this). And physical pixels are called physical > pixels (in cc) or just pixels (in ui/aura). So I mildly prefer not adding a > third naming scheme. > > Examples: > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/dip_... > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/quads/render_pa... > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre... > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/layer_im... > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/quads/draw_quad... > > >> >> Finally, you could include the reference to the Dimensions doc here, if >> you think it'd be a useful reference to avoid having to describe all >> this to n00bs like me. ;) >> >> >> https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/client/an... >> blimp/client/android/blimp_view.h:55: // A reference to the Java object >> which owns this class. >> On 2015/09/03 06:33:21, David Trainor (OOO) wrote: >> >>> On 2015/09/03 00:49:27, Wez wrote: >>> > nit: No need for "A" here. >>> > >>> > So this reference has no effect on GC...? i.e. we're not creating a >>> > reference-loop here? >>> >> >> We are. That's on purpose though. >>> >> >> OK - sounds like the ownership graph needs clarifying, then? >> >> >> https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/BU... >> File blimp/common/BUILD.gn (right): >> >> >> https://chromiumcodereview.appspot.com/1295243003/diff/300001/blimp/common/BU... >> blimp/common/BUILD.gn:8: # are finalized or are pushed from the server >> component. >> On 2015/09/03 06:33:22, David Trainor (OOO) wrote: >> >>> On 2015/09/03 00:49:28, Wez wrote: >>> > nit: Do we have a bug filed to track that work, that you can refer >>> >> to here? >> >> We don't, but I'll file one. >>> >> >> Thanks. :) >> >> >> https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... >> File blimp/client/android/blimp_jni_registrar.h (right): >> >> >> https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... >> blimp/client/android/blimp_jni_registrar.h:15: // the native Java VM >> code. >> I'd suggest: >> >> "Registers native method hooks with the Java runtime specified by |env|. >> Returns false if registration fails, in which case native methods are >> unavailable to Java." >> >> It wasn't clear to me reading the code whether the hooks actually retain >> references to |env| or whether this method is self-contained? >> >> >> https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... >> File blimp/client/android/blimp_library_loader.cc (right): >> >> >> https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... >> blimp/client/android/blimp_library_loader.cc:97: // calling these >> methods makes us future-proof for underlying JNI refactors. >> nit: I think: >> >> "Although we only need to register JNI for base/ and blimp/, this >> follows the general Chrome for Android pattern, to be future-proof >> against future changes to JNI." >> >> >> https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... >> File blimp/client/android/blimp_view.h (right): >> >> >> https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... >> blimp/client/android/blimp_view.h:31: // (device pixels) to pixels. >> Confused - I thought dp was Device/Density Independent Pixels, i.e. >> logical pixels, and "pixel" was actual physical pixels? >> >> >> https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... >> blimp/client/android/blimp_view.h:35: const gfx::Size& display_size, >> Why not name these |real_size| and |size|, for consistency with >> android.view.Display, OR name them |full_display_size| and >> |display_size| if you want to be a little more descriptive, say? >> "physical" size is confusing since it implies the distinction is between >> dp and px, for example! >> >> >> https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... >> File blimp/client/compositor/blimp_compositor_android.cc (right): >> >> >> https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... >> blimp/client/compositor/blimp_compositor_android.cc:86: // tile when in >> portrait mode. This helps worst case scroll/raster >> Why do we only do this optimization for real_size_supported_? >> >> >> https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... >> blimp/client/compositor/blimp_compositor_android.cc:88: int >> right_tile_width = (portrait_width_ - 1) % default_tile_size + 1; >> nit: Brackets around the % to make it clear that it's (A % B) +1, not A >> & (B + 1) >> >> >> https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... >> blimp/client/compositor/blimp_compositor_android.cc:93: 1; >> Isn't ceil(A/B) - 1 in practice floor(A/B) in this situation? i.e. we >> can just say: >> >> full_tile_count = A / B; >> >> and then say that we're going to expand the tile size to ensure we have >> only that many tiles? >> >> >> https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... >> blimp/client/compositor/blimp_compositor_android.cc:100: // Round up to >> nearest 32. >> nit: Explain why - sounds like this is for GPU efficiency? >> >> >> https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... >> File blimp/client/compositor/blimp_compositor_android.h (right): >> >> >> https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... >> blimp/client/compositor/blimp_compositor_android.h:33: // (device >> pixels) to pixels. >> See comment on the BlimpView and note that you've called dp_to_pixel >> "device_scale_factor" here, which is confusing! >> >> >> https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... >> blimp/client/compositor/blimp_compositor_android.h:50: // (device >> pixels) to pixels. >> This is a copy of the comment on Create() - it doesn't describe any of >> the actual parameters of this ctor, at least not using their actual >> names! >> >> https://chromiumcodereview.appspot.com/1295243003/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... File blimp/client/android/blimp_jni_registrar.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... blimp/client/android/blimp_jni_registrar.h:15: // the native Java VM code. On 2015/09/03 18:26:11, Wez wrote: > I'd suggest: > > "Registers native method hooks with the Java runtime specified by |env|. Returns > false if registration fails, in which case native methods are unavailable to > Java." > > It wasn't clear to me reading the code whether the hooks actually retain > references to |env| or whether this method is self-contained? Ah yeah it's self contained (at least as far as our automatically generated JNI code is concerned). https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... File blimp/client/android/blimp_library_loader.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... blimp/client/android/blimp_library_loader.cc:97: // calling these methods makes us future-proof for underlying JNI refactors. On 2015/09/03 18:26:11, Wez wrote: > nit: I think: > > "Although we only need to register JNI for base/ and blimp/, this follows the > general Chrome for Android pattern, to be future-proof against future changes to > JNI." Done. https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... File blimp/client/android/blimp_view.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... blimp/client/android/blimp_view.h:31: // (device pixels) to pixels. On 2015/09/03 18:26:11, Wez wrote: > Confused - I thought dp was Device/Density Independent Pixels, i.e. logical > pixels, and "pixel" was actual physical pixels? It is. Sorry for the poor wording. https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/an... blimp/client/android/blimp_view.h:35: const gfx::Size& display_size, On 2015/09/03 18:26:11, Wez wrote: > Why not name these |real_size| and |size|, for consistency with > android.view.Display, OR name them |full_display_size| and |display_size| if you > want to be a little more descriptive, say? "physical" size is confusing since it > implies the distinction is between dp and px, for example! Good idea. Will do. https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... File blimp/client/compositor/blimp_compositor_android.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.cc:86: // tile when in portrait mode. This helps worst case scroll/raster On 2015/09/03 18:26:11, Wez wrote: > Why do we only do this optimization for real_size_supported_? That's the way render_widget_compositor is handling it. I'm doing the same thing here. If we want to change this further let's look into doing it for both places separately. https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.cc:88: int right_tile_width = (portrait_width_ - 1) % default_tile_size + 1; On 2015/09/03 18:26:11, Wez wrote: > nit: Brackets around the % to make it clear that it's (A % B) +1, not A & (B + > 1) Done. https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.cc:93: 1; On 2015/09/03 18:26:11, Wez wrote: > Isn't ceil(A/B) - 1 in practice floor(A/B) in this situation? i.e. we can just > say: > > full_tile_count = A / B; > > and then say that we're going to expand the tile size to ensure we have only > that many tiles? Yeah that should work. Especially since we already know that we overflow a bit. https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.cc:100: // Round up to nearest 32. On 2015/09/03 18:26:11, Wez wrote: > nit: Explain why - sounds like this is for GPU efficiency? Done. https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... File blimp/client/compositor/blimp_compositor_android.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.h:33: // (device pixels) to pixels. On 2015/09/03 18:26:11, Wez wrote: > See comment on the BlimpView and note that you've called dp_to_pixel > "device_scale_factor" here, which is confusing! Done. https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.h:50: // (device pixels) to pixels. On 2015/09/03 18:26:11, Wez wrote: > This is a copy of the comment on Create() - it doesn't describe any of the > actual parameters of this ctor, at least not using their actual names! Yeah my bad. Fixed this.
lgtm
The CQ bit was checked by wez@chromium.org
lgtm Just a couple more comment issues, that we can address via a follow-up CL. https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... File blimp/client/compositor/blimp_compositor_android.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/320001/blimp/client/co... blimp/client/compositor/blimp_compositor_android.cc:86: // tile when in portrait mode. This helps worst case scroll/raster On 2015/09/03 19:06:00, David Trainor (OOO) wrote: > On 2015/09/03 18:26:11, Wez wrote: > > Why do we only do this optimization for real_size_supported_? > > That's the way render_widget_compositor is handling it. I'm doing the same > thing here. If we want to change this further let's look into doing it for both > places separately. Acknowledged. https://chromiumcodereview.appspot.com/1295243003/diff/340001/blimp/client/an... File blimp/client/android/blimp_view.h (right): https://chromiumcodereview.appspot.com/1295243003/diff/340001/blimp/client/an... blimp/client/android/blimp_view.h:31: // pixels) to px. This still describes "dp" as "device pixels", when they are DIPs (Device-Independent Pixels), and you're still not indicating whether |real_size| and |size| are in DIPs or Pixels. https://chromiumcodereview.appspot.com/1295243003/diff/340001/blimp/client/co... File blimp/client/compositor/blimp_compositor.cc (right): https://chromiumcodereview.appspot.com/1295243003/diff/340001/blimp/client/co... blimp/client/compositor/blimp_compositor.cc:40: : device_scale_factor_(dp_to_px) {} Rename the member as well?
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, sievers@chromium.org, dpranke@chromium.org, reed@google.com Link to the patchset: https://chromiumcodereview.appspot.com/1295243003/#ps340001 (title: "Address final nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1295243003/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1295243003/340001
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/e9c78dddbb858895a13ed8b22c53a07d63a9ff1b Cr-Commit-Position: refs/heads/master@{#347223} |
