Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2717363003:
Add Physical Web WebUI Default Url Icon (Closed)
|
Created:
3 years, 9 months ago by Ran Modified:
3 years, 8 months ago CC:
cco3, chromium-reviews, marq+watch_chromium.org, mmocny, noyau+watch_chromium.org, oshima+watch_chromium.org, pkl (ping after 24h if needed), sdefresne+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionShow a default URL icon instead of a broken image for urls with no icon.
BUG=682244
Review-Url: https://codereview.chromium.org/2717363003
Cr-Commit-Position: refs/heads/master@{#460648}
Committed: https://chromium.googlesource.com/chromium/src/+/23c9664fcc2ffd6bdcffbca5355ba4cd083f7060
Patch Set 1 #
Total comments: 3
Patch Set 2 : create differnt versions of img #
Total comments: 12
Patch Set 3 : use image directly #
Total comments: 2
Patch Set 4 : fix #Patch Set 5 : rebase #
Messages
Total messages: 54 (31 generated)
Description was changed from ========== Add default icon BUG= ========== to ========== Add default url icons instead of showing a broken image. BUG=682244 ==========
Description was changed from ========== Add default url icons instead of showing a broken image. BUG=682244 ========== to ========== make a default url icon instead of showing a broken image. BUG=682244 ==========
The CQ bit was checked by ranj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== make a default url icon instead of showing a broken image. BUG=682244 ========== to ========== Show a default URL icon instead of a broken image. BUG=682244 ==========
ranj@chromium.org changed reviewers: + cco3@chromium.org, mattreynolds@chromium.org
Description was changed from ========== Show a default URL icon instead of a broken image. BUG=682244 ========== to ========== Show a default URL icon instead of a broken image for pages with no icon. BUG=682244 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Commit message should have 50 column limit and not include a full stop (period).
Is that png file already in the tree elsewhere? If so, it'd be good if there were a way to share it instead of copying it.
https://codereview.chromium.org/2717363003/diff/1/components/physical_web/web... File components/physical_web/webui/resources/physical_web.js (right): https://codereview.chromium.org/2717363003/diff/1/components/physical_web/web... components/physical_web/webui/resources/physical_web.js:33: for (var i = 0; i < images.length; i++) { I know you are working with some old version of jquery, but you should be able to do: $('img').foreach(function() { $(this).addEventListener(...) });
https://codereview.chromium.org/2717363003/diff/1/chrome/browser/ui/webui/phy... File chrome/browser/ui/webui/physical_web/physical_web_ui.cc (right): https://codereview.chromium.org/2717363003/diff/1/chrome/browser/ui/webui/phy... chrome/browser/ui/webui/physical_web/physical_web_ui.cc:34: IDR_PHYSICAL_WEB_UI_LINK_ICON); Image resources should be added to the components/resources/default_%d_percent directories (you'll need 100%, 200%, and 300% versions of the icon). You should also create components/resources/physical_web_ui_scaled_resources.grdp and define the resource ID there. Add the new .grdp file to components/resources/components_scaled_resources.grd See https://cs.corp.google.com/clankium/src/components/resources/flags_ui_scaled_... for an example. If you haven't already, run tools/resources/optimize_png_files.sh on the new image resources. If they were already in Chrome they may already be optimized, but it doesn't hurt to run it again. https://codereview.chromium.org/2717363003/diff/1/components/physical_web/web... File components/physical_web/webui/resources/physical_web.js (right): https://codereview.chromium.org/2717363003/diff/1/components/physical_web/web... components/physical_web/webui/resources/physical_web.js:32: var images = document.getElementsByTagName('img'); Can we make this only match favicon images? If we add any other error-prone images to the page this will catch them too.
mattreynolds@google.com changed reviewers: + mattreynolds@google.com
https://codereview.chromium.org/2717363003/diff/20001/components/physical_web... File components/physical_web/webui/resources/physical_web.js (right): https://codereview.chromium.org/2717363003/diff/20001/components/physical_web... components/physical_web/webui/resources/physical_web.js:32: var images = document.getElementsByTagName('img'); Would getElementsByName work here to get only the favicons? https://codereview.chromium.org/2717363003/diff/20001/components/physical_web... components/physical_web/webui/resources/physical_web.js:36: this.src = "chrome://physical-web/link.png"; Use a relative path here (relative to the webui URL chrome://physical-web/) Also, shouldn't this be ic_link_grey600_36dp.png?
mmocny@chromium.org changed reviewers: + mmocny@chromium.org
Agree with Conley & Matt's comments, added a few more. Overall, I think we could save some of these types of the tasks if we move to using polymer custom elements and material design -- but I think we can hold off on that for a while since this patch addresses the specific problem. https://codereview.chromium.org/2717363003/diff/20001/components/physical_web... File components/physical_web/webui/resources/physical_web.js (right): https://codereview.chromium.org/2717363003/diff/20001/components/physical_web... components/physical_web/webui/resources/physical_web.js:27: bodyContainer.style.visibility = 'visible'; Nit: could you create a CSS class for hidden items, and then add/remove the class instead of manually adjusting styles like this? You can use Element.classList.add/remove to this: https://developer.mozilla.org/en/docs/Web/API/Element/classList https://codereview.chromium.org/2717363003/diff/20001/components/physical_web... components/physical_web/webui/resources/physical_web.js:31: function addErrorHandler() { Please rename: `assignImageLoadErrorHandlers` https://codereview.chromium.org/2717363003/diff/20001/components/physical_web... components/physical_web/webui/resources/physical_web.js:32: var images = document.getElementsByTagName('img'); On 2017/03/02 01:02:39, Matt Reynolds wrote: > Would getElementsByName work here to get only the favicons? Personally I like using document.querySelector() (or document.querySelectorAll() in this case) since it uses the same syntax as your CSS selectors. Its trivially less efficient (for this size UI). https://developer.mozilla.org/en-US/docs/Web/API/Element/querySelector https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelector https://codereview.chromium.org/2717363003/diff/20001/components/physical_web... components/physical_web/webui/resources/physical_web.js:32: var images = document.getElementsByTagName('img'); Nit: chrome support `let` keyword now, which is more sane. It doesn't matter much for `images` variable, but your use of `i` and `img` later on are not scoped the way you expect.
On 2017/02/28 22:10:49, cco3 wrote: > https://codereview.chromium.org/2717363003/diff/1/components/physical_web/web... > File components/physical_web/webui/resources/physical_web.js (right): > > https://codereview.chromium.org/2717363003/diff/1/components/physical_web/web... > components/physical_web/webui/resources/physical_web.js:33: for (var i = 0; i < > images.length; i++) { > I know you are working with some old version of jquery, but you should be able > to do: > > $('img').foreach(function() { > $(this).addEventListener(...) > }); It's in chrome/android/java/res/drawable-mdpi/ic_link_grey600_36dp.png. We have to copy it so that it can be used in ios as well.
https://codereview.chromium.org/2717363003/diff/120001/components/physical_we... File components/physical_web/webui/resources/physical_web.js (right): https://codereview.chromium.org/2717363003/diff/120001/components/physical_we... components/physical_web/webui/resources/physical_web.js:37: '../../../../components/resources/default_100_percent/ppp/ic_link_grey600_36dp.png'; ppp? should be physical_web?
On 2017/03/06 19:33:34, mattreynolds wrote: > https://codereview.chromium.org/2717363003/diff/120001/components/physical_we... > File components/physical_web/webui/resources/physical_web.js (right): > > https://codereview.chromium.org/2717363003/diff/120001/components/physical_we... > components/physical_web/webui/resources/physical_web.js:37: > '../../../../components/resources/default_100_percent/ppp/ic_link_grey600_36dp.png'; > ppp? should be physical_web? Sorry it was a experimental. I was replying Conley and not mean to sent for review. Seems physical_web.js cannot access file components/resources/default_100_percent/physical_web/ic_link_grey600_36dp.png, it can only access files under chrome://physical-web, or chrome://somename/*.png which has a url. I can't find any sample that uses scaled image directly in html/js, the closest example is https://cs.chromium.org/chromium/src/components/neterror/resources/ It puts the resource files under the same directory. Can I do it the same way?
I can't find any sample that uses scaled image in html, use the image directly. As the icon is relatively small, I don't think it matters in most of the case. Refer sample: https://cs.corp.google.com/clankium/src/components/resources/ntp_tiles_resour... https://codereview.chromium.org/2717363003/diff/20001/components/physical_web... File components/physical_web/webui/resources/physical_web.js (right): https://codereview.chromium.org/2717363003/diff/20001/components/physical_web... components/physical_web/webui/resources/physical_web.js:27: bodyContainer.style.visibility = 'visible'; On 2017/03/02 17:13:35, mmocny wrote: > Nit: could you create a CSS class for hidden items, and then add/remove the > class instead of manually adjusting styles like this? > > You can use Element.classList.add/remove to this: > https://developer.mozilla.org/en/docs/Web/API/Element/classList Done. https://codereview.chromium.org/2717363003/diff/20001/components/physical_web... components/physical_web/webui/resources/physical_web.js:31: function addErrorHandler() { On 2017/03/02 17:13:35, mmocny wrote: > Please rename: `assignImageLoadErrorHandlers` Done. https://codereview.chromium.org/2717363003/diff/20001/components/physical_web... components/physical_web/webui/resources/physical_web.js:32: var images = document.getElementsByTagName('img'); On 2017/03/02 17:13:35, mmocny wrote: > Nit: chrome support `let` keyword now, which is more sane. > > It doesn't matter much for `images` variable, but your use of `i` and `img` > later on are not scoped the way you expect. Done. https://codereview.chromium.org/2717363003/diff/20001/components/physical_web... components/physical_web/webui/resources/physical_web.js:32: var images = document.getElementsByTagName('img'); On 2017/03/02 01:02:39, Matt Reynolds wrote: > Would getElementsByName work here to get only the favicons? Done. https://codereview.chromium.org/2717363003/diff/20001/components/physical_web... components/physical_web/webui/resources/physical_web.js:32: var images = document.getElementsByTagName('img'); On 2017/03/02 17:13:35, mmocny wrote: > On 2017/03/02 01:02:39, Matt Reynolds wrote: > > Would getElementsByName work here to get only the favicons? > > Personally I like using document.querySelector() (or document.querySelectorAll() > in this case) since it uses the same syntax as your CSS selectors. Its > trivially less efficient (for this size UI). > > https://developer.mozilla.org/en-US/docs/Web/API/Element/querySelector > https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelector Done. https://codereview.chromium.org/2717363003/diff/20001/components/physical_web... components/physical_web/webui/resources/physical_web.js:36: this.src = "chrome://physical-web/link.png"; On 2017/03/02 01:02:39, Matt Reynolds wrote: > Use a relative path here (relative to the webui URL chrome://physical-web/) > > Also, shouldn't this be ic_link_grey600_36dp.png? Done.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
Approach seems reasonable! However, I have no experience with exposing image resources to webui and I would like someone with expertise to sign off to see if this is the right way. https://codereview.chromium.org/2717363003/diff/160001/components/physical_we... File components/physical_web/webui/resources/physical_web.js (right): https://codereview.chromium.org/2717363003/diff/160001/components/physical_we... components/physical_web/webui/resources/physical_web.js:39: let img = e.getElementsByTagName('img')[0]; Could we just change the querySelectorAll to do this for us? You can use nested selectors, try this: '.physicalWebIcon > img'
Patchset #4 (id:180001) has been deleted
https://codereview.chromium.org/2717363003/diff/160001/components/physical_we... File components/physical_web/webui/resources/physical_web.js (right): https://codereview.chromium.org/2717363003/diff/160001/components/physical_we... components/physical_web/webui/resources/physical_web.js:39: let img = e.getElementsByTagName('img')[0]; On 2017/03/23 20:58:39, mmocny wrote: > Could we just change the querySelectorAll to do this for us? You can use nested > selectors, try this: > > '.physicalWebIcon > img' Done.
lgtm for webui changes. Hoping someone can confirm there is no alternative way to reuse existing image assets.
lgtm
ranj@chromium.org changed reviewers: - mattreynolds@google.com
ranj@chromium.org changed reviewers: + bauerb@chromium.org - cco3@chromium.org
ranj@chromium.org changed reviewers: + jochen@chromium.org - bauerb@chromium.org
Description was changed from ========== Show a default URL icon instead of a broken image for pages with no icon. BUG=682244 ========== to ========== Show a default URL icon instead of a broken image for urls with no icon. BUG=682244 ==========
+jochen
what part would you like me to review? I'd hope that everything can be covered by ios or physical_web owners?
+jochen for chrome/browser/ui/webui/physical_web/physical_web_ui.cc components/resources/physical_web_ui_resources.grdp Sorry for confuse
can you please also send a CL that makes /components/physical_web/OWNERS owners for the grdp file and the c/b/ui/webui/physical_web directory? rubberstamp lgtm
ranj@chromium.org changed reviewers: + eugenebut@chromium.org
+eugenebut for ios/chrome/browser/ui/webui/physical_web_ui.cc
ios lgtm
The CQ bit was checked by ranj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ranj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, mmocny@chromium.org, eugenebut@chromium.org, mattreynolds@chromium.org Link to the patchset: https://codereview.chromium.org/2717363003/#ps220001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1490845133203610,
"parent_rev": "528e9669fe7420bc8299c323298c1cae6fc1d166", "commit_rev":
"23c9664fcc2ffd6bdcffbca5355ba4cd083f7060"}
Message was sent while issue was closed.
Description was changed from ========== Show a default URL icon instead of a broken image for urls with no icon. BUG=682244 ========== to ========== Show a default URL icon instead of a broken image for urls with no icon. BUG=682244 Review-Url: https://codereview.chromium.org/2717363003 Cr-Commit-Position: refs/heads/master@{#460648} Committed: https://chromium.googlesource.com/chromium/src/+/23c9664fcc2ffd6bdcffbca5355b... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:220001) as https://chromium.googlesource.com/chromium/src/+/23c9664fcc2ffd6bdcffbca5355b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
