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

Status: RESOLVED FIXED

(Firefox :: Theme, enhancement, P5)

Description

[Jared Wein [:jaws] (please needinfo? me)]

+++ 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. ;)

Comment 1

[Tim Nguyen :ntim]

Looks like Jeremy's patch ended up here: https://reviewboard-hg.mozilla.org/gecko/rev/a8d32830f53c

Comment 2

[Jeremy Lempereur [:o0ignition0o]]

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 ? :)

Comment 3

[Jeremy Lempereur [:o0ignition0o]]

Attachment: Bug 1481470 - Stop using xlink:href in SVG (just use href) in remaining parts of the codebase r?jaws

MozReview-Commit-ID: E10bVjuTkQZ

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 5

[Jeremy Lempereur [:o0ignition0o]]

You were right, putting everything back to LF does the trick. 

I ll try to submit a new patch tonight :D

Comment 6

[Jeremy Lempereur [:o0ignition0o]]

Ok it looks much better now :D

Comment 7

[Jeremy Lempereur [:o0ignition0o]]

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 :)

Comment 8

[Andrew Overholt [: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!

Comment 9

[:Gijs (he/him)]

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

Comment 10

[Jeremy Lempereur [:o0ignition0o]]

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! :)

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 13

[Silke Hofmann]

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

Comment 14

[Silke Hofmann]

Attachment: Script to generate patch

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.

Comment 15

[Silke Hofmann]

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! :)

Comment 16

[(inactive) Nihanth Subramanya [:nhnt11]]

(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! :-)

Comment 17

[Silke Hofmann]

Attachment: Script to generate patch - v2

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! :)

Comment 18

[Silke Hofmann]

Attachment: Bug 1481470 - Stop using xlink:href in SVG in docs/.

Comment 19

[Silke Hofmann]

Attachment: Bug 1481470 - Stop using xlink:href in SVG in browser/.

Depends on D117203

Comment 20

[Silke Hofmann]

Attachment: Bug 1481470 - Stop using xlink:href in SVG in devtools/.

Depends on D117204

Comment 21

[Silke Hofmann]

Attachment: Bug 1481470 - Stop using xlink:href in SVG in dom/.

Depends on D117205

Comment 22

[Silke Hofmann]

Attachment: Bug 1481470 - Stop using xlink:href in SVG in toolkit/.

Depends on D117206

Comment 23

[Silke Hofmann]

Attachment: Bug 1481470 - Stop using xlink:href in SVG in services/.

Depends on D117207

Comment 24

[Silke Hofmann]

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.

Comment 25

[(inactive) Nihanth Subramanya [:nhnt11]]

(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!

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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

Comment 27

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/00fd6a554c23
https://hg.mozilla.org/mozilla-central/rev/aa2d06d17d5b
https://hg.mozilla.org/mozilla-central/rev/24f0b71ae997
https://hg.mozilla.org/mozilla-central/rev/2b5c3a210260
https://hg.mozilla.org/mozilla-central/rev/623dff6d1af4
https://hg.mozilla.org/mozilla-central/rev/f531f12e5c35

Comment 28

[(inactive) Nihanth Subramanya [:nhnt11]]

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.