Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 1 month ago by bshe Modified:
5 years, 1 month ago Reviewers:
Sergey Ulanov, mfomitchev, sky CC:
chromium-reviews, chromium-apps-reviews_chromium.org, tfarina, extensions-reviews_chromium.org, no sievers Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShow error message for desktop capture related APIs on Aura Android
BUG=557285
Committed: https://crrev.com/7fb069de3fd47a263587fd24f5136745a1d2c53e
Cr-Commit-Position: refs/heads/master@{#360831}
Patch Set 1 #
Total comments: 4
Patch Set 2 : reviews #
Total comments: 2
Patch Set 3 : reviews #
Created: 5 years, 1 month ago
Depends on Patchset: Messages
Total messages: 16 (5 generated)
bshe@chromium.org changed reviewers: + mfomitchev@chromium.org
Hi Mikhail. Do you mind to take a look at this CL? Thanks!
LGTM with a nit
Description was changed from ========== Show error message for desktop capture related APIs on Aura Android BUG=557285 ========== to ========== Show error message for desktop capture related APIs on Aura Android BUG=557285 ==========
https://codereview.chromium.org/1457743002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc (right): https://codereview.chromium.org/1457743002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:131: #if defined(TOOLKIT_VIEWS) && !defined(OS_ANDROID) || defined(OS_MACOSX) Can you please add braces here so that it is more explicit/easier to read? https://codereview.chromium.org/1457743002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:131: #if defined(TOOLKIT_VIEWS) && !defined(OS_ANDROID) || defined(OS_MACOSX) Note that Dan is adding a new ANDROID_JAVA_UI define - see https://codereview.chromium.org/1459793002/. It's meant to be used in chrome/ (it really means "browser UI done in native Android Java"). Once this lands, we should switch USE_AURA && ANDROID in chrome/ to ANDROID_JAVA_UI
bshe@chromium.org changed reviewers: + sergeyu@chromium.org, sky@chromium.org
thanks! +owners: sky@ for chrome/browser/ui/views/desktop_media_picker_views.cc sergeyu@ for chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc https://codereview.chromium.org/1457743002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc (right): https://codereview.chromium.org/1457743002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:131: #if defined(TOOLKIT_VIEWS) && !defined(OS_ANDROID) || defined(OS_MACOSX) On 2015/11/19 15:56:55, mfomitchev wrote: > Can you please add braces here so that it is more explicit/easier to read? Done. https://codereview.chromium.org/1457743002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:131: #if defined(TOOLKIT_VIEWS) && !defined(OS_ANDROID) || defined(OS_MACOSX) On 2015/11/19 15:56:55, mfomitchev wrote: > Note that Dan is adding a new ANDROID_JAVA_UI define - see > https://codereview.chromium.org/1459793002/. It's meant to be used in chrome/ > (it really means "browser UI done in native Android Java"). Once this lands, we > should switch USE_AURA && ANDROID in chrome/ to ANDROID_JAVA_UI Added a TODO.
LGTM https://codereview.chromium.org/1457743002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_media_picker_views.cc (right): https://codereview.chromium.org/1457743002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_media_picker_views.cc:55: #if defined(OS_WIN) || defined(OS_ANDROID) Is it worth a NOT_IMPLEMENTED() for android?
lgtm
Thanks all for review! https://codereview.chromium.org/1457743002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_media_picker_views.cc (right): https://codereview.chromium.org/1457743002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_media_picker_views.cc:55: #if defined(OS_WIN) || defined(OS_ANDROID) On 2015/11/19 20:40:41, sky wrote: > Is it worth a NOT_IMPLEMENTED() for android? Done.
The CQ bit was checked by bshe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfomitchev@chromium.org, sergeyu@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1457743002/#ps40001 (title: "reviews")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1457743002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1457743002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7fb069de3fd47a263587fd24f5136745a1d2c53e Cr-Commit-Position: refs/heads/master@{#360831}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1563843002/ by bshe@chromium.org. The reason for reverting is: We dont need to support Aura Android at current stage. Revert related code.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
