Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1866753008:
Add ability to linkify hashes on isolate server (Closed)
|
Created:
4 years, 8 months ago by kjlubick Modified:
4 years, 8 months ago Reviewers:
M-A Ruel CC:
chromium-reviews, infra-reviews+luci-py_chromium.org Base URL:
https://chromium.googlesource.com/external/github.com/luci/luci-py@master Target Ref:
refs/heads/master Project:
luci-py Visibility:
Public. |
DescriptionAdd ability to linkify hashes and display images on isolate server.
Links are clickable and added to the JSON:
https://screenshot.googleplex.com/hqNZU3XHQBQ
Images are displayed (for common, browser handled images like png, jpg, gif, bmp):
https://screenshot.googleplex.com/y0o5hgjQN9C
Text (e.g. scripts) is also displayed:
https://screenshot.googleplex.com/5D62SBO9QNA
BUG=
Committed: https://github.com/luci/luci-py/commit/c768b0b456add9ee056f2da69fe7dc17f5091ae4
Patch Set 1 #Patch Set 2 : Also add content sniffing #
Total comments: 11
Patch Set 3 : Address feedback #
Total comments: 19
Patch Set 4 : Address feedback round 2 #
Total comments: 12
Patch Set 5 : Minor cleanups #
Total comments: 4
Patch Set 6 : Add comment #
Total comments: 1
Patch Set 7 : Fix content Noneness #
Messages
Total messages: 22 (8 generated)
kjlubick@google.com changed reviewers: + maruel@chromium.org
Description was changed from ========== Add ability to linkify hashes on isolate server BUG= ========== to ========== Add ability to linkify hashes and display images on isolate server. Links are clickable and added to the JSON: https://screenshot.googleplex.com/hqNZU3XHQBQ Images are displayed (for common, browser handled images like png, jpg, gif, bmp): https://screenshot.googleplex.com/y0o5hgjQN9C Text (e.g. scripts) is also displayed: https://screenshot.googleplex.com/5D62SBO9QNA BUG= ==========
PTAL
looks good! few tweaks https://codereview.chromium.org/1866753008/diff/20001/appengine/isolate/handl... File appengine/isolate/handlers_frontend.py (right): https://codereview.chromium.org/1866753008/diff/20001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:158: # TODO(maruel): Add back once Web UI authentication is switched to OAuth2. Remove this while at it, not needed. https://codereview.chromium.org/1866753008/diff/20001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:164: # TODO(maruel): Refactor into a function. Can you make it a function now that it is used at 3 different places (including handlers_endpoints_v1). Put it in model.py. https://codereview.chromium.org/1866753008/diff/20001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:175: 2 empty lines between file level symbols https://codereview.chromium.org/1866753008/diff/20001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:219: '<div style="font-family:monospace;white-space:pre-wrap;">%s</div>' Looks like this is not strictly necessary from what I could see by browsing a bash script. https://codereview.chromium.org/1866753008/diff/20001/appengine/isolate/templ... File appengine/isolate/templates/browse.html (right): https://codereview.chromium.org/1866753008/diff/20001/appengine/isolate/templ... appengine/isolate/templates/browse.html:143: <iframe id="content" class="use_all_space" sandbox="allow-same-origin allow-popups" src="content?namespace={{namespace}}&digest={{digest}}"> Why allow-popups?
https://codereview.chromium.org/1866753008/diff/20001/appengine/isolate/handl... File appengine/isolate/handlers_frontend.py (right): https://codereview.chromium.org/1866753008/diff/20001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:158: # TODO(maruel): Add back once Web UI authentication is switched to OAuth2. On 2016/04/13 at 20:32:09, M-A Ruel wrote: > Remove this while at it, not needed. Done. https://codereview.chromium.org/1866753008/diff/20001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:158: # TODO(maruel): Add back once Web UI authentication is switched to OAuth2. On 2016/04/13 at 20:32:09, M-A Ruel wrote: > Remove this while at it, not needed. Done. https://codereview.chromium.org/1866753008/diff/20001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:164: # TODO(maruel): Refactor into a function. On 2016/04/13 at 20:32:09, M-A Ruel wrote: > Can you make it a function now that it is used at 3 different places (including handlers_endpoints_v1). Put it in model.py. I put it in model.py and used it twice here. I couldn't figure out a nice way to stick it in handlers_endpoints_v1 because of how the various pieces of the lookups were entangled. https://codereview.chromium.org/1866753008/diff/20001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:175: On 2016/04/13 at 20:32:09, M-A Ruel wrote: > 2 empty lines between file level symbols Done. https://codereview.chromium.org/1866753008/diff/20001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:219: '<div style="font-family:monospace;white-space:pre-wrap;">%s</div>' On 2016/04/13 at 20:32:09, M-A Ruel wrote: > Looks like this is not strictly necessary from what I could see by browsing a bash script. I'll add a comment that mirrors this, but I don't add this for the styling, but to use a div instead of a pre. I can't put anchor tags inside of a pre. https://codereview.chromium.org/1866753008/diff/20001/appengine/isolate/templ... File appengine/isolate/templates/browse.html (right): https://codereview.chromium.org/1866753008/diff/20001/appengine/isolate/templ... appengine/isolate/templates/browse.html:143: <iframe id="content" class="use_all_space" sandbox="allow-same-origin allow-popups" src="content?namespace={{namespace}}&digest={{digest}}"> On 2016/04/13 at 20:32:09, M-A Ruel wrote: > Why allow-popups? If allow-popups isn't set to true, I can't make the default behavior for the link "open in new tab", which I think is a good default.
https://codereview.chromium.org/1866753008/diff/40001/appengine/isolate/handl... File appengine/isolate/handlers_frontend.py (right): https://codereview.chromium.org/1866753008/diff/40001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:167: self.abort(404, 'Unable to retrieve the entry') alignment https://codereview.chromium.org/1866753008/diff/40001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:178: content = None This can be removed. https://codereview.chromium.org/1866753008/diff/40001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:180: raw_data = None You need to move up above the condition, otherwise it'd fail line 188 with variable uninitialized. https://codereview.chromium.org/1866753008/diff/40001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:189: key = model.get_entry_key(namespace, digest) In this case, the entity is effectively loaded twice, this is suboptimal. https://codereview.chromium.org/1866753008/diff/40001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:209: # If we don't wrap this in html, browsers will put content in a pre Thanks for the explanation, I didn't know this. https://codereview.chromium.org/1866753008/diff/40001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:218: r'<a target="_blank" href="browse?namespace=default-gzip' + use the namespcae instead of hardcoding 'default-gzip' https://codereview.chromium.org/1866753008/diff/40001/appengine/isolate/model.py File appengine/isolate/model.py (right): https://codereview.chromium.org/1866753008/diff/40001/appengine/isolate/model... appengine/isolate/model.py:136: 2 lines between file level symbols https://codereview.chromium.org/1866753008/diff/40001/appengine/isolate/model... appengine/isolate/model.py:138: """Returns the content from either memcache or datastore. Add a note that it doesn't return content from GCS. https://codereview.chromium.org/1866753008/diff/40001/appengine/isolate/model... appengine/isolate/model.py:141: Raises ValueError if the hash_key is invalid.""" """ on a separate line https://codereview.chromium.org/1866753008/diff/40001/appengine/isolate/templ... File appengine/isolate/templates/browse.html (right): https://codereview.chromium.org/1866753008/diff/40001/appengine/isolate/templ... appengine/isolate/templates/browse.html:47: <iframe id="content" class="use_all_space" sandbox="allow-same-origin allow-popups" src="content?namespace={{namespace}}&digest={{digest}}"> src="/content?... ?
https://codereview.chromium.org/1866753008/diff/40001/appengine/isolate/handl... File appengine/isolate/handlers_frontend.py (right): https://codereview.chromium.org/1866753008/diff/40001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:167: self.abort(404, 'Unable to retrieve the entry') On 2016/04/14 at 14:05:06, M-A Ruel wrote: > alignment Done. https://codereview.chromium.org/1866753008/diff/40001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:178: content = None On 2016/04/14 at 14:05:06, M-A Ruel wrote: > This can be removed. Done. https://codereview.chromium.org/1866753008/diff/40001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:180: raw_data = None On 2016/04/14 at 14:05:06, M-A Ruel wrote: > You need to move up above the condition, otherwise it'd fail line 188 with variable uninitialized. Good catch. Done. https://codereview.chromium.org/1866753008/diff/40001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:189: key = model.get_entry_key(namespace, digest) On 2016/04/14 at 14:05:06, M-A Ruel wrote: > In this case, the entity is effectively loaded twice, this is suboptimal. Done. entity is returned as the second argument. https://codereview.chromium.org/1866753008/diff/40001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:218: r'<a target="_blank" href="browse?namespace=default-gzip' + On 2016/04/14 at 14:05:06, M-A Ruel wrote: > use the namespcae instead of hardcoding 'default-gzip' Done. https://codereview.chromium.org/1866753008/diff/40001/appengine/isolate/model.py File appengine/isolate/model.py (right): https://codereview.chromium.org/1866753008/diff/40001/appengine/isolate/model... appengine/isolate/model.py:136: On 2016/04/14 at 14:05:06, M-A Ruel wrote: > 2 lines between file level symbols Done. https://codereview.chromium.org/1866753008/diff/40001/appengine/isolate/model... appengine/isolate/model.py:138: """Returns the content from either memcache or datastore. On 2016/04/14 at 14:05:06, M-A Ruel wrote: > Add a note that it doesn't return content from GCS. Done. https://codereview.chromium.org/1866753008/diff/40001/appengine/isolate/model... appengine/isolate/model.py:141: Raises ValueError if the hash_key is invalid.""" On 2016/04/14 at 14:05:06, M-A Ruel wrote: > """ on a separate line Done. https://codereview.chromium.org/1866753008/diff/40001/appengine/isolate/templ... File appengine/isolate/templates/browse.html (right): https://codereview.chromium.org/1866753008/diff/40001/appengine/isolate/templ... appengine/isolate/templates/browse.html:47: <iframe id="content" class="use_all_space" sandbox="allow-same-origin allow-popups" src="content?namespace={{namespace}}&digest={{digest}}"> On 2016/04/14 at 14:05:06, M-A Ruel wrote: > src="/content?... ? Done.
https://codereview.chromium.org/1866753008/diff/60001/appengine/isolate/handl... File appengine/isolate/handlers_frontend.py (right): https://codereview.chromium.org/1866753008/diff/60001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:158: u'onload': '', not needed, remove https://codereview.chromium.org/1866753008/diff/60001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:217: r'<a target="_blank" href="browse?namespace=%s' % namespace + href="/browse... https://codereview.chromium.org/1866753008/diff/60001/appengine/isolate/model.py File appengine/isolate/model.py (right): https://codereview.chromium.org/1866753008/diff/60001/appengine/isolate/model... appengine/isolate/model.py:139: """Returns the content from either memcache or datastore. Returns the content from either memcache or datastore, when stored inline. https://codereview.chromium.org/1866753008/diff/60001/appengine/isolate/model... appengine/isolate/model.py:143: The first argument in the tuple is the content, the second is either Returns: tuple(content, ContentEntry) At most only one of the two is set. https://codereview.chromium.org/1866753008/diff/60001/appengine/isolate/templ... File appengine/isolate/templates/browse.html (right): https://codereview.chromium.org/1866753008/diff/60001/appengine/isolate/templ... appengine/isolate/templates/browse.html:8: height: 95%; why? I prefer static margins to 95% https://codereview.chromium.org/1866753008/diff/60001/appengine/isolate/templ... appengine/isolate/templates/browse.html:27: min-height: 80%; sort
https://codereview.chromium.org/1866753008/diff/60001/appengine/isolate/handl... File appengine/isolate/handlers_frontend.py (right): https://codereview.chromium.org/1866753008/diff/60001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:158: u'onload': '', On 2016/04/14 at 19:22:36, M-A Ruel wrote: > not needed, remove Done. https://codereview.chromium.org/1866753008/diff/60001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:217: r'<a target="_blank" href="browse?namespace=%s' % namespace + On 2016/04/14 at 19:22:36, M-A Ruel wrote: > href="/browse... Done. https://codereview.chromium.org/1866753008/diff/60001/appengine/isolate/model.py File appengine/isolate/model.py (right): https://codereview.chromium.org/1866753008/diff/60001/appengine/isolate/model... appengine/isolate/model.py:139: """Returns the content from either memcache or datastore. On 2016/04/14 at 19:22:36, M-A Ruel wrote: > Returns the content from either memcache or datastore, when stored inline. Done. https://codereview.chromium.org/1866753008/diff/60001/appengine/isolate/model... appengine/isolate/model.py:143: The first argument in the tuple is the content, the second is either On 2016/04/14 at 19:22:36, M-A Ruel wrote: > Returns: > tuple(content, ContentEntry) > At most only one of the two is set. Done. https://codereview.chromium.org/1866753008/diff/60001/appengine/isolate/templ... File appengine/isolate/templates/browse.html (right): https://codereview.chromium.org/1866753008/diff/60001/appengine/isolate/templ... appengine/isolate/templates/browse.html:8: height: 95%; On 2016/04/14 at 19:22:36, M-A Ruel wrote: > why? I prefer static margins to 95% I found that height of 100% always required some vertical scrolling, likely due to the appengine footer. I could possibly do absolute positioning to make it static. https://codereview.chromium.org/1866753008/diff/60001/appengine/isolate/templ... appengine/isolate/templates/browse.html:27: min-height: 80%; On 2016/04/14 at 19:22:36, M-A Ruel wrote: > sort Sorted.
lgtm with 2 issues. https://codereview.chromium.org/1866753008/diff/80001/appengine/isolate/handl... File appengine/isolate/handlers_frontend.py (right): https://codereview.chromium.org/1866753008/diff/80001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:177: raw_data = None It is not needed, remove. https://codereview.chromium.org/1866753008/diff/80001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:194: del self.response.headers['Content-Type'] Why do you delete it before setting it? Remove.
https://codereview.chromium.org/1866753008/diff/80001/appengine/isolate/handl... File appengine/isolate/handlers_frontend.py (right): https://codereview.chromium.org/1866753008/diff/80001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:177: raw_data = None On 2016/04/14 at 20:02:40, M-A Ruel wrote: > It is not needed, remove. I thought you said it would crash with uninitialized error? https://codereview.chromium.org/1866753008/diff/80001/appengine/isolate/handl... appengine/isolate/handlers_frontend.py:194: del self.response.headers['Content-Type'] On 2016/04/14 at 20:02:39, M-A Ruel wrote: > Why do you delete it before setting it? Remove. I found that it actually doesn't work if I don't delete it. If I don't delete it, there are two Content-Type headers instead of one. I've made a comment for it.
The CQ bit was checked by kjlubick@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org Link to the patchset: https://codereview.chromium.org/1866753008/#ps100001 (title: "Add comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1866753008/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1866753008/100001
https://codereview.chromium.org/1866753008/diff/100001/appengine/isolate/hand... File appengine/isolate/handlers_frontend.py (right): https://codereview.chromium.org/1866753008/diff/100001/appengine/isolate/hand... appengine/isolate/handlers_frontend.py:177: raw_data = None You confused this, it's only content that needs to be set as None.
The CQ bit was unchecked by maruel@chromium.org
The CQ bit was checked by kjlubick@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org Link to the patchset: https://codereview.chromium.org/1866753008/#ps120001 (title: "Fix content Noneness")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1866753008/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1866753008/120001
Message was sent while issue was closed.
Description was changed from ========== Add ability to linkify hashes and display images on isolate server. Links are clickable and added to the JSON: https://screenshot.googleplex.com/hqNZU3XHQBQ Images are displayed (for common, browser handled images like png, jpg, gif, bmp): https://screenshot.googleplex.com/y0o5hgjQN9C Text (e.g. scripts) is also displayed: https://screenshot.googleplex.com/5D62SBO9QNA BUG= ========== to ========== Add ability to linkify hashes and display images on isolate server. Links are clickable and added to the JSON: https://screenshot.googleplex.com/hqNZU3XHQBQ Images are displayed (for common, browser handled images like png, jpg, gif, bmp): https://screenshot.googleplex.com/y0o5hgjQN9C Text (e.g. scripts) is also displayed: https://screenshot.googleplex.com/5D62SBO9QNA BUG= Committed: https://github.com/luci/luci-py/commit/c768b0b456add9ee056f2da69fe7dc17f5091ae4 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://github.com/luci/luci-py/commit/c768b0b456add9ee056f2da69fe7dc17f5091ae4 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
