Closed
Bug 1284842
Opened 8 years ago
Closed 8 years ago
Reps: handle URLs uniformly
Categories
(DevTools :: Shared Components, enhancement, P1)
DevTools
Shared Components
Tracking
(firefox50 affected, firefox51 fixed)
People
(Reporter: linclark, Assigned: dalimil)
References
Details
(Keywords: good-first-bug, Whiteboard: [devtools-html][good first bug])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
Honza
:
review+
|
Details | Diff | Splinter Review |
Urls seem to be handled in different ways - window shows full url: http://example.com/ - document doesn't include protocol: www.facebook.com - stylesheet only includes file name: contentSearchUI.css I believe all of these (and any others) should be full urls.
Updated•8 years ago
|
Blocks: devtools-html-2
Updated•8 years ago
|
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [devtools-html] [triage] → [devtools-html]
Reporter | ||
Updated•8 years ago
|
Whiteboard: [devtools-html] → [devtools-html][good first bug]
Comment 1•8 years ago
|
||
Hi Lin, I would like to work on this, It would be great if you could tell me where to start with
Reporter | ||
Comment 2•8 years ago
|
||
Sorry, I just realized this bug isn't ready yet. We have a few issues with patches that are going to land soon which need to be done before this. I've attached that as a blocker.
Depends on: 1257552
Assignee | ||
Comment 3•8 years ago
|
||
Test for the relevant rep modules have already been created so this bug wasn't blocked anymore and also had a relatively simple solution. Push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=668af6412833
Updated•8 years ago
|
Iteration: --- → 50.4 - Aug 1
Priority: P2 → P1
Reporter | ||
Comment 4•8 years ago
|
||
Comment on attachment 8774705 [details] [diff] [review] Bug1284842.patch - rev1 Review of attachment 8774705 [details] [diff] [review]: ----------------------------------------------------------------- Rather than using crop string, we should be passing this through a URL function. It may be that we need to update the url function to process it properly.
Attachment #8774705 -
Flags: review?(lclark) → review-
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Lin Clark [:linclark] from comment #4) > Rather than using crop string, we should be passing this through a URL > function. It may be that we need to update the url function to process it > properly. There are a few URL related functions in rep-utils but they are mostly used for splitting. I thought we just want to display the url as is (possibly cropped)? Or do you want me to just create a simple function parseURLToDisplay(urlText) in rep-utils that for now would only call cropString() - but would provide an abstraction to change it later? BTW, I also checked object-with-url.js and it doesn't even crop the url.
Flags: needinfo?(lclark)
Reporter | ||
Comment 6•8 years ago
|
||
I think it would be best to compile a list here of all of the places in reps where urls are shown and what function is used to show the URL. We can make a better decision once we have that information.
Flags: needinfo?(lclark)
Assignee | ||
Comment 7•8 years ago
|
||
OK, here are the ones that I was able to find (looking at 'master' and ignoring the new patch): window.js -> cropString(grip.preview.url) document.js -> getFileName(grip.preview.location) stylesheet.js -> getFileName(grip.preview.url) object-with-url.js -> displays 'grip.preview.url' (without any cropping) rep-utils.js -> contain URL parsing and splitting methods that are used in reps
Updated•8 years ago
|
Iteration: 50.4 - Aug 1 → 51.1 - Aug 15
Assignee | ||
Comment 8•8 years ago
|
||
Honza, could you take a look at this? I feel this has got stuck...
Flags: needinfo?(odvarko)
Comment 9•8 years ago
|
||
It's great you are working on this! I was quickly discussing this with Lin and we agree that there should be a function that's used to prepare the URL string for rendering. Just like you are mentioning in comment #5. Implementation of that function can be simple cropping (or some kind of smarter URL cropping?) and it should be used at all the places you summarized in comment #7. The function will be nice abstraction and so, we can improve it later if needed. I can review the patch. Honza
Flags: needinfo?(odvarko)
Assignee | ||
Comment 10•8 years ago
|
||
OK, I created the new function in rep-utils. Push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ab6641ecfbb
Attachment #8774705 -
Attachment is obsolete: true
Attachment #8781487 -
Flags: review?(odvarko)
Updated•8 years ago
|
Iteration: 51.1 - Aug 15 → 51.2 - Aug 29
Comment 11•8 years ago
|
||
[good first bug] whiteboard -> keyword mass change
Keywords: good-first-bug
Comment 12•8 years ago
|
||
Comment on attachment 8781487 [details] [diff] [review] Bug1284842.patch - rev2 Review of attachment 8781487 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me now! R+ assuming the try is green Honza
Attachment #8781487 -
Flags: review?(odvarko) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 13•8 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/fx-team/rev/1f04326ada82 Reps: Handle URLs uniformly. r=Honza
Keywords: checkin-needed
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1f04326ada82
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•