Closed Bug 1481470 Opened 5 years ago Closed 2 years ago

Stop using xlink:href in SVG (just use href) in remaining parts of the codebase

Categories

(Firefox :: Theme, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
91 Branch
Tracking Status
firefox91 --- fixed

People

(Reporter: jaws, Assigned: silke, Mentored)

References

Details

(Keywords: good-first-bug, perf)

Attachments

(8 files, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #1359030 +++

(Bug 1359030 focused on only /browser/ and /toolkit/. This bug will cover the remaining parts of the codebase)

We've supported plain 'href' in SVG instead of 'xlink:href' for a long time now. Our internal code should get rid of the:

  xmlns:xlink="http://www.w3.org/1999/xlink" 

declarations, and switch to using plain 'href'.

Marking as perf since there's then a bit less parsing, but the idea that it would make a noticeable perf difference is a bit dubious. ;)
Depends on: 1359030
Looks like Jeremy's patch ended up here: https://reviewboard-hg.mozilla.org/gecko/rev/a8d32830f53c
Wow this is very bad ^^'

I'm trying to figure out how to use arc and phabricator properly (haven't succeeded yet), but it seems like my python script to rewrite stuff is changing each file instead of only the lines it is supposed to.

I'll try to figure out how the other lines changed (to revert it) and submit a more decent patch.

For now the diff is here somehow https://phabricator.services.mozilla.com/D3410

It will be much better once I figure out the new tooling ^^'

I'm also adding the contents of the (very ugly, but almost working) python script for debug purposes (the files variable is the hg status second column output (so each files i have changed with the find/replace command)

> import os
> string = "xlink"
> import re
> def rewrite_lines(file, index):
>     if file[index].strip() == "xmlns:xlink='http://www.w3.org/1999/xlink'>"  or file[index].strip() == 'xmlns:xlink="http://www.w3.org/1999/xlink">':
>         file[index-1] = file[index-1].replace("\n", ">\n")
>         print "lonely line with >"
>         print file[index-1]
>         del file[index]
>         return file
>     if file[index].strip() == "xmlns:xlink='http://www.w3.org/1999/xlink'" or file[index].strip() == 'xmlns:xlink="http://www.w3.org/1999/xlink"':
>         print "lonely line"
>         print file[index]
>         del file[index]
>         return file
>     new_line = file[index].replace(" xmlns:xlink='http://www.w3.org/1999/xlink'", "")
>     new_line = new_line.replace(' xmlns:xlink="http://www.w3.org/1999/xlink"', "")
>     print "in the middle of a line"
>     print new_line
>     return file
>
> for file in files:
>     with open(file, 'r+') as content:
>         useful_lines = []
>         lines = []
>         i = 0
>         for line in content:
>             lines.append(line)
>             if re.search("xlink", line):
>                 from pprint import pprint
>                 pprint(line)
>                 useful_lines.append(i)
>             i = i+1
>         if len(useful_lines) == 1:
>           new_content = rewrite_lines(lines, useful_lines.pop())
>           content.seek(0)
>           content.write("".join(new_content))
>           content.truncate()

I think the problem is I need to rewrite the whole file, and python is probably changing something in the process. I wonder if it has something to do with CRLF (I m running windows) or encoding.

Any thoughts Jared ? :)
Flags: needinfo?(jaws)
Yes, I would guess it is changing line endings too. It might also be something else in your toolchain that is changing the line endings. If you run `hg histedit` and choose "edit" to edit the changeset, then run `hg diff path/to/file` you should see the diff for that file which will most likely show every line has been changed. Then if you open a text editor and change each line ending to remove the carriage return, save the file, and then run `hg diff` do you see that the diff has gotten considerably smaller?
Flags: needinfo?(jaws) → needinfo?(jeremy.lempereur)
You were right, putting everything back to LF does the trick. 

I ll try to submit a new patch tonight :D
Flags: needinfo?(jeremy.lempereur)
Ok it looks much better now :D
As I'm trying to split the review request into separate folders, I have identified several folders : 

devtools
dom
gfx
image
js
layout
testing

The testing folder involves a lot of changes, so I might want to split it too.

Andrew, since you're the dom triage owner, and since I'd like to start with this folder, could you please point me to someone who could review the changes I've made in the .svg files? 

Thanks a lot :)
Flags: needinfo?(overholt)
(In reply to Jeremy Lempereur [:o0ignition0o] from comment #7)
> Andrew, since you're the dom triage owner, and since I'd like to start with
> this folder, could you please point me to someone who could review the
> changes I've made in the .svg files? 

I think you could ask Peter Van Der Beken. Thanks for doing this!
Flags: needinfo?(overholt)

Hey Jeremy, what's the status of this bug? Are you still interested in driving this over the line? :-)

Flags: needinfo?(jeremy.lempereur)

Oh I totally forgot about this one :/

Sure I need to finish fixing an important one first, and I'll keep this one in mind (since it will now appear on top of my dashboard it should be better !)

Thanks for reminding me of it! :)

Flags: needinfo?(jeremy.lempereur)

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Assignee: jeremy.lempereur → nobody
Status: ASSIGNED → NEW
Assignee: nobody → jeremy.lempereur
Status: NEW → ASSIGNED

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Assignee: jeremy.lempereur → nobody
Status: ASSIGNED → NEW
Assignee: nobody → silke
Status: NEW → ASSIGNED
Attached file Script to generate patch (obsolete) —

I wrote this script to generate the patch. I'll provide more information this weekend, but in the meantime I just wanted to have this uploaded already.

Attachment #9222887 - Flags: feedback?(nhnt11)
Attachment #9222886 - Attachment description: WIP: Bug 1481470 - Stop using xlink:href in SVG (just use href) in remaining parts of the codebase → Bug 1481470 - Stop using xlink:href in SVG (just use href) in remaining parts of the codebase

Some info about the attached script: It finds all SVG files recursively, starting from the given directory. Each file is searched for the Strings to be modified, and the found Strings are replaced. By default the script only prints the changed files as a dry run. With -w the changes are written to the files.

Let me know if there is any additional info I can provide! :)

(In reply to Silke Hofmann from comment #15)

Some info about the attached script: It finds all SVG files recursively, starting from the given directory. Each file is searched for the Strings to be modified, and the found Strings are replaced. By default the script only prints the changed files as a dry run. With -w the changes are written to the files.

Let me know if there is any additional info I can provide! :)

Nice, I love the dry-run feature! :-)

v2 includes two changes in the original script:

  • Files that are in test/ directories are now omitted.
  • When an attribute is removed from the svg tag, empty lines and lines only containing ">" are cleaned up.

Thanks for the input & feedback! :)

Attachment #9222887 - Attachment is obsolete: true
Attachment #9222887 - Flags: feedback?(nhnt11)
Attachment #9225078 - Flags: feedback?(nhnt11)
Attachment #9222886 - Attachment is obsolete: true

Depends on D117203

Depends on D117204

Depends on D117205

Depends on D117206

Depends on D117207

I’ve made the following changes since the last (now abandoned) commit: 


  • The script now only searches & modifies SVG files that are not located in a test directory. The logic for identifying all test directories has been taken from Searchfox.
  • The regex patterns used for find & replace the Strings have been extended to remove any empty lines, and lines only containing “>” after modification.
  • As for SVG files within the “docs” directory, it turns out the xlink variable is called xl, so additional logic has been implemented to also find and replace xl:href & xmlns:xl="http://www.w3.org/1999/xlink”.
  • I’ve extended the script to identify all top level directories that contain relevant SVG files: “docs”, "browser”, “devtools", "dom", "toolkit" and ”services”. I’ve created one new commit for each directory. Across all directories, only 58 SVG files are touched now.

(In reply to Silke Hofmann from comment #24)

Awesome. I skimmed all of your patches and they look good. At the moment, they are all marked as WIP, could you add me as a reviewer on each of them and submit for review? I can help find additional reviewers as well where necessary.

I’ve made the following changes since the last (now abandoned) commit: 


  • The script now only searches & modifies SVG files that are not located in a test directory. The logic for identifying all test directories has been taken from Searchfox.

  • The regex patterns used for find & replace the Strings have been extended to remove any empty lines, and lines only containing “>” after modification.

  • As for SVG files within the “docs” directory, it turns out the xlink variable is called xl, so additional logic has been implemented to also find and replace xl:href & xmlns:xl="http://www.w3.org/1999/xlink”.

🧐👍
I just realized, since we are removing the xlink namespace we should not only remove xlink:href but any other attribute name that is prefixed with xlink:, i.e. xlink:foo. But going through the patches, it's fairly trivial to verify that there are no other values of foo than "href", so we are good.

  • I’ve extended the script to identify all top level directories that contain relevant SVG files: “docs”, "browser”, “devtools", "dom", "toolkit" and ”services”. I’ve created one new commit for each directory. Across all directories, only 58 SVG files are touched now.

Wow, that's two orders of magnitude smaller than if we include tests. Could you file a follow up bug, blocking this one, for investigating removing this from SVGs in test directories where possible?

We should also have a follow-up bug to investigate adding some kind of linting that prevents us from introducing more of these in the future. I'm not aware of whether we have any infrastructure to lint SVGs, but that can be figured out by whoever works on it when the time comes. 😉

Thanks for working on this!

Flags: needinfo?(silke)
Attachment #9225968 - Attachment description: WIP: Bug 1481470 - Stop using xlink:href in SVG in docs/. → Bug 1481470 - Stop using xlink:href in SVG in docs/.
Attachment #9225969 - Attachment description: WIP: Bug 1481470 - Stop using xlink:href in SVG in browser/. → Bug 1481470 - Stop using xlink:href in SVG in browser/.
Attachment #9225970 - Attachment description: WIP: Bug 1481470 - Stop using xlink:href in SVG in devtools/. → Bug 1481470 - Stop using xlink:href in SVG in devtools/.
Attachment #9225971 - Attachment description: WIP: Bug 1481470 - Stop using xlink:href in SVG in dom/. → Bug 1481470 - Stop using xlink:href in SVG in dom/.
Attachment #9225972 - Attachment description: WIP: Bug 1481470 - Stop using xlink:href in SVG in toolkit/. → Bug 1481470 - Stop using xlink:href in SVG in toolkit/.
Attachment #9225973 - Attachment description: WIP: Bug 1481470 - Stop using xlink:href in SVG in services/. → Bug 1481470 - Stop using xlink:href in SVG in services/.
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/00fd6a554c23
Stop using xlink:href in SVG in docs/. r=nhnt11,firefox-source-docs-reviewers,championshuttler
https://hg.mozilla.org/integration/autoland/rev/aa2d06d17d5b
Stop using xlink:href in SVG in browser/. r=nhnt11,desktop-theme-reviewers,harry
https://hg.mozilla.org/integration/autoland/rev/24f0b71ae997
Stop using xlink:href in SVG in devtools/. r=nhnt11,bomsy
https://hg.mozilla.org/integration/autoland/rev/2b5c3a210260
Stop using xlink:href in SVG in dom/. r=nhnt11,nika
https://hg.mozilla.org/integration/autoland/rev/623dff6d1af4
Stop using xlink:href in SVG in toolkit/. r=nhnt11,desktop-theme-reviewers,harry
https://hg.mozilla.org/integration/autoland/rev/f531f12e5c35
Stop using xlink:href in SVG in services/. r=nhnt11,leplatrem
Flags: needinfo?(silke)

Comment on attachment 9225078 [details]
Script to generate patch - v2

A little late to set this flag but just wanted to acknowledge for posterity that I've seen this script and also collaborated on it with Silke off of the bug and it looks good.

Attachment #9225078 - Flags: feedback?(nhnt11) → feedback+
You need to log in before you can comment on or make changes to this bug.