Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 1 month ago by bshe Modified:
5 years, 1 month ago Reviewers:
palmer, msw, Peter Kasting, mfomitchev CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable url_formatter::ElideUrl and url_formatter::ElideHost for aura Android
Aura Android uses text_utils_gfx.cc and GetStringWidthF is
implemented in the file. So it is safe to use
url_formatter::ElideUrl and url_formatter::ElideHost for
aura Android.
BUG=507792
Committed: https://crrev.com/cd2dd0a028b2f2efe3f1053eb34091e30e0d1548
Cr-Commit-Position: refs/heads/master@{#358442}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : reviews #Patch Set 5 : Enable ElideUrl for aura Android #
Total comments: 2
Patch Set 6 : ' #Patch Set 7 : change gfx::ElideText too #
Total comments: 12
Patch Set 8 : #Patch Set 9 : #Patch Set 10 : refer to bug #
Created: 5 years, 1 month ago
Depends on Patchset: Messages
Total messages: 34 (11 generated)
bshe@chromium.org changed reviewers: + mfomitchev@chromium.org
Hi Mikhail. Do you mind to take a look at this simple CL? ElideUrl is not implemented on Android platform yet. So simply disable them for now.
Do you know why ElideUrl doesn't work on Android? Would it be worth using gfx::ElideText in the meantime instead of completely stubbing out the behavior?
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
On 2015/11/02 19:47:37, mfomitchev wrote: > Do you know why ElideUrl doesn't work on Android? > Would it be worth using gfx::ElideText in the meantime instead of completely > stubbing out the behavior? I think it is because this function isn't implemented on Android platform: https://code.google.com/p/chromium/codesearch/#chromium/src/ui/gfx/text_utils... I didn't try gfx::ElideText. But looks like it is used in ElideUrl implementation. So I ifdefed some code in elide_url instead to fix the issue. It should work fine. But since we can't run binary yet. I am not entirely sure. Anyway, PTAL.
Description was changed from ========== Disable URL elide on Android ElideUrl is not implemented on Android yet. So disable elide url for Android platform for now. BUG=507792 ========== to ========== Use gfx::ElideText to elide URL for aura on Android Once we have GetStringWidthF implemented on Android platform, we can switch to use url_formatter::ElideUrl and url_formatter::ElideHost. BUG=507792 ==========
On 2015/11/03 14:08:43, bshe wrote: > On 2015/11/02 19:47:37, mfomitchev wrote: > > Do you know why ElideUrl doesn't work on Android? > > Would it be worth using gfx::ElideText in the meantime instead of completely > > stubbing out the behavior? > > I think it is because this function isn't implemented on Android platform: > https://code.google.com/p/chromium/codesearch/#chromium/src/ui/gfx/text_utils... > I didn't try gfx::ElideText. But looks like it is used in ElideUrl > implementation. So > I ifdefed some code in elide_url instead to fix the issue. It should work fine. > But since we can't > run binary yet. I am not entirely sure. Anyway, PTAL. Looking at gfx::ElideText, it actually uses GetStringWidthF on Android, so it it seems like it wouldn't work correctly. Given that, it's unclear to me why gfx::ElideText is even made available.. Either way, considering this - making more functions, like ElideUrl available on (non-Aura) Android is probably not the right thing to do. Also, GetStringWidthF is actually available on Aura Android: it's implemented in text_utils_skia.cc. Note Aura Android ends up using text_utils_skia.cc, and not text_utils_androidf.cc: https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/BUILD.gn&q=... Given this, maybe we can just enable relevant functionality on Aura Android, keeping it disabled on non-Aura Android?
Description was changed from ========== Use gfx::ElideText to elide URL for aura on Android Once we have GetStringWidthF implemented on Android platform, we can switch to use url_formatter::ElideUrl and url_formatter::ElideHost. BUG=507792 ========== to ========== Enable url_formatter::ElideUrl and url_formatter::ElideHost for aura Android Aura Android uses text_utils_gfx.cc and GetStringWidthF is implemented in the file. So it is safe to use url_formatter::ElideUrl and url_formatter::ElideHost for aura Android. BUG=507792 ==========
Patchset #5 (id:120001) has been deleted
On 2015/11/03 15:46:59, mfomitchev wrote: > On 2015/11/03 14:08:43, bshe wrote: > > On 2015/11/02 19:47:37, mfomitchev wrote: > > > Do you know why ElideUrl doesn't work on Android? > > > Would it be worth using gfx::ElideText in the meantime instead of completely > > > stubbing out the behavior? > > > > I think it is because this function isn't implemented on Android platform: > > > https://code.google.com/p/chromium/codesearch/#chromium/src/ui/gfx/text_utils... > > I didn't try gfx::ElideText. But looks like it is used in ElideUrl > > implementation. So > > I ifdefed some code in elide_url instead to fix the issue. It should work > fine. > > But since we can't > > run binary yet. I am not entirely sure. Anyway, PTAL. > > Looking at gfx::ElideText, it actually uses GetStringWidthF on Android, so it it > seems like it wouldn't work correctly. Given that, it's unclear to me why > gfx::ElideText is even made available.. Either way, considering this - making > more functions, like ElideUrl available on (non-Aura) Android is probably not > the right thing to do. > > Also, GetStringWidthF is actually available on Aura Android: it's implemented in > text_utils_skia.cc. Note Aura Android ends up using text_utils_skia.cc, and not > text_utils_androidf.cc: > https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/BUILD.gn&q=... > > Given this, maybe we can just enable relevant functionality on Aura Android, > keeping it disabled on non-Aura Android? Good catch. I assumed gfx::ElideText works since it has an Android implementation. I must go deeper... Since we use text_utils_skia for aura Android, looks like the Cl could be much simpler. PTAL
LGTM with a nit >I must go deeper... Just make sure you don't lose your totem! ;) https://codereview.chromium.org/1428593004/diff/140001/components/url_formatt... File components/url_formatter/BUILD.gn (right): https://codereview.chromium.org/1428593004/diff/140001/components/url_formatt... components/url_formatter/BUILD.gn:27: deps += [ "//ui/gfx" ] This is not your issue, but it is a bit weird that here were are adding a dependency conditionally, and below we are removing the same dependency conditionally. Do you mind making it consistent? Personally I prefer removing the dependency, since the condition is easier to read, but I think either is fine.
bshe@chromium.org changed reviewers: + palmer@chromium.org
Thanks Mikhail. +palmer for OWNER https://codereview.chromium.org/1428593004/diff/140001/components/url_formatt... File components/url_formatter/BUILD.gn (right): https://codereview.chromium.org/1428593004/diff/140001/components/url_formatt... components/url_formatter/BUILD.gn:27: deps += [ "//ui/gfx" ] On 2015/11/03 17:03:14, mfomitchev wrote: > This is not your issue, but it is a bit weird that here were are adding a > dependency conditionally, and below we are removing the same dependency > conditionally. Do you mind making it consistent? Personally I prefer removing > the dependency, since the condition is easier to read, but I think either is > fine. Done.
bshe@chromium.org changed reviewers: + msw@chromium.org
+msw for ui/gfx It looks like we need to do aura check in elide_url.cc too. Thanks!
https://codereview.chromium.org/1428593004/diff/180001/components/url_formatt... File components/url_formatter/BUILD.gn (right): https://codereview.chromium.org/1428593004/diff/180001/components/url_formatt... components/url_formatter/BUILD.gn:4: import("//build/config/ui.gni") nit: add a blank line above. https://codereview.chromium.org/1428593004/diff/180001/components/url_formatt... components/url_formatter/BUILD.gn:27: if (is_android && !use_aura) { Consider: if (!is_android || use_aura) { deps += [ "//ui/gfx" ] } https://codereview.chromium.org/1428593004/diff/180001/components/url_formatt... components/url_formatter/BUILD.gn:28: deps -= [ "//ui/gfx" ] Shouldn't the GYP file have corresponding changes? url_formatter.gyp
https://codereview.chromium.org/1428593004/diff/180001/ui/gfx/text_elider.cc File ui/gfx/text_elider.cc (right): https://codereview.chromium.org/1428593004/diff/180001/ui/gfx/text_elider.cc#... ui/gfx/text_elider.cc:247: render_text->set_truncate_length(5000); Is there some kMaxSomethingSomething that this code and Canvas::SizeStringFloat? It'd be nice to have a named constant, and to have a way of ensuring that this code and Canvas do indeed stay in sync.
@msw https://codereview.chromium.org/1428593004/diff/180001/components/url_formatt... File components/url_formatter/BUILD.gn (right): https://codereview.chromium.org/1428593004/diff/180001/components/url_formatt... components/url_formatter/BUILD.gn:4: import("//build/config/ui.gni") On 2015/11/03 23:55:33, msw wrote: > nit: add a blank line above. Done. https://codereview.chromium.org/1428593004/diff/180001/components/url_formatt... components/url_formatter/BUILD.gn:27: if (is_android && !use_aura) { On 2015/11/03 23:55:33, msw wrote: > Consider: if (!is_android || use_aura) { deps += [ "//ui/gfx" ] } Done. https://codereview.chromium.org/1428593004/diff/180001/components/url_formatt... components/url_formatter/BUILD.gn:28: deps -= [ "//ui/gfx" ] On 2015/11/03 23:55:33, msw wrote: > Shouldn't the GYP file have corresponding changes? url_formatter.gyp For aura Android, we will only support gn build. The gyp version of aura Android wont generate correct ninja file currently I believe. So I left gyp file unchanged. My understanding is that gyp will eventually be replaced. So it is probably fine to only change gn file?
@palmer https://codereview.chromium.org/1428593004/diff/180001/ui/gfx/text_elider.cc File ui/gfx/text_elider.cc (right): https://codereview.chromium.org/1428593004/diff/180001/ui/gfx/text_elider.cc#... ui/gfx/text_elider.cc:247: render_text->set_truncate_length(5000); On 2015/11/04 00:46:07, palmer wrote: > Is there some kMaxSomethingSomething that this code and Canvas::SizeStringFloat? > It'd be nice to have a named constant, and to have a way of ensuring that this > code and Canvas do indeed stay in sync. It looks like they are already out of sync. I cant find anything in SizeStringFloat that uses 5000. I am not familiar with this area of code. But I find this constant here: https://code.google.com/p/chromium/codesearch/#chromium/src/ui/gfx/render_tex... It looks like the new value from comment. But I am not quite sure. Do you think we should unify the two constants?
lgtm https://codereview.chromium.org/1428593004/diff/180001/components/url_formatt... File components/url_formatter/BUILD.gn (right): https://codereview.chromium.org/1428593004/diff/180001/components/url_formatt... components/url_formatter/BUILD.gn:28: deps -= [ "//ui/gfx" ] On 2015/11/04 14:38:00, bshe wrote: > On 2015/11/03 23:55:33, msw wrote: > > Shouldn't the GYP file have corresponding changes? url_formatter.gyp > > For aura Android, we will only support gn build. The gyp version of aura Android > wont generate correct ninja file currently I believe. So I left gyp file > unchanged. My understanding is that gyp will eventually be replaced. So it is > probably fine to only change gn file? Hmm, it's a gray area, I's update GYP too, but it probably doesn't matter. https://codereview.chromium.org/1428593004/diff/180001/ui/gfx/text_elider.cc File ui/gfx/text_elider.cc (right): https://codereview.chromium.org/1428593004/diff/180001/ui/gfx/text_elider.cc#... ui/gfx/text_elider.cc:247: render_text->set_truncate_length(5000); On 2015/11/04 14:38:39, bshe wrote: > On 2015/11/04 00:46:07, palmer wrote: > > Is there some kMaxSomethingSomething that this code and > Canvas::SizeStringFloat? > > It'd be nice to have a named constant, and to have a way of ensuring that this > > code and Canvas do indeed stay in sync. > > > It looks like they are already out of sync. > > I cant find anything in SizeStringFloat that uses 5000. > I am not familiar with this area of code. But I find this > constant here: > https://code.google.com/p/chromium/codesearch/#chromium/src/ui/gfx/render_tex... > It looks like the new value from comment. But I am not quite sure. Do > you think we should unify the two constants? Well, that's the updated layout limit for RenderText (HarfBuzz). The old RenderTextWin (Uniscribe) layout limit was likely the basis for this 5000 value. You could simply remove this set_truncate_length call to use the underlying default 10k value, and it'd probably just make the eliding of long strings slower (that's hopefully not happening in a tight loop anyway). You could also leave that for a follow-up CL, since it seems like a tangential change.
https://codereview.chromium.org/1428593004/diff/180001/components/url_formatt... File components/url_formatter/BUILD.gn (right): https://codereview.chromium.org/1428593004/diff/180001/components/url_formatt... components/url_formatter/BUILD.gn:28: deps -= [ "//ui/gfx" ] On 2015/11/04 19:16:11, msw wrote: > On 2015/11/04 14:38:00, bshe wrote: > > On 2015/11/03 23:55:33, msw wrote: > > > Shouldn't the GYP file have corresponding changes? url_formatter.gyp > > > > For aura Android, we will only support gn build. The gyp version of aura > Android > > wont generate correct ninja file currently I believe. So I left gyp file > > unchanged. My understanding is that gyp will eventually be replaced. So it is > > probably fine to only change gn file? > > Hmm, it's a gray area, I's update GYP too, but it probably doesn't matter. Right. Updated GYP file since it is pretty trivial to do so in this case. https://codereview.chromium.org/1428593004/diff/180001/ui/gfx/text_elider.cc File ui/gfx/text_elider.cc (right): https://codereview.chromium.org/1428593004/diff/180001/ui/gfx/text_elider.cc#... ui/gfx/text_elider.cc:247: render_text->set_truncate_length(5000); On 2015/11/04 19:16:11, msw wrote: > On 2015/11/04 14:38:39, bshe wrote: > > On 2015/11/04 00:46:07, palmer wrote: > > > Is there some kMaxSomethingSomething that this code and > > Canvas::SizeStringFloat? > > > It'd be nice to have a named constant, and to have a way of ensuring that > this > > > code and Canvas do indeed stay in sync. > > > > > > It looks like they are already out of sync. > > > > I cant find anything in SizeStringFloat that uses 5000. > > I am not familiar with this area of code. But I find this > > constant here: > > > https://code.google.com/p/chromium/codesearch/#chromium/src/ui/gfx/render_tex... > > It looks like the new value from comment. But I am not quite sure. Do > > you think we should unify the two constants? > > Well, that's the updated layout limit for RenderText (HarfBuzz). The old > RenderTextWin (Uniscribe) layout limit was likely the basis for this 5000 value. > You could simply remove this set_truncate_length call to use the underlying > default 10k value, and it'd probably just make the eliding of long strings > slower (that's hopefully not happening in a tight loop anyway). You could also > leave that for a follow-up CL, since it seems like a tangential change. A follow-up CL makes sense. I have created crbug.com/551660 to track it.
lgtm
On 2015/11/04 22:40:42, msw wrote: > lgtm friendly ping palmer@
> > You could simply remove this set_truncate_length call to use the underlying > > default 10k value, and it'd probably just make the eliding of long strings > > slower (that's hopefully not happening in a tight loop anyway). You could also > > leave that for a follow-up CL, since it seems like a tangential change. > > A follow-up CL makes sense. I have created crbug.com/551660 to track it. Please update the comment in text_elider.cc to refer to that bug. LGTM once you do that.
bshe@chromium.org changed reviewers: + pkasting@chromium.org
Added reference to the bug. And looks like I need another owner for components/url_formatter +pkasting Thanks
LGTM
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, palmer@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/1428593004/#ps240001 (title: "refer to bug")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1428593004/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1428593004/240001
Message was sent while issue was closed.
Committed patchset #10 (id:240001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/cd2dd0a028b2f2efe3f1053eb34091e30e0d1548 Cr-Commit-Position: refs/heads/master@{#358442}
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:240001) has been created in https://codereview.chromium.org/1562833002/ by bshe@chromium.org. The reason for reverting is: We dont need to support Aura Android at current stage. Revert related code.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
