URLs in Page Info Media tab should be LTR

RESOLVED FIXED in Firefox 3.7a5

Status

()

Firefox
Page Info Window
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

({rtl})

Trunk
Firefox 3.7a5
Points:
---
Dependency tree / graph
Bug Flags:
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

The URLs in the Media tab of the Page Info dialog should appear LTR even if the UI is RTL.

This is similar to bug 478430.
Flags: in-litmus?
(Assignee)

Comment 1

9 years ago
Created attachment 376617 [details] [diff] [review]
Patch (v1)
Attachment #376617 - Flags: review?(gavin.sharp)
Comment on attachment 376617 [details] [diff] [review]
Patch (v1)

>diff --git a/browser/base/content/pageinfo/pageInfo.js b/browser/base/content/pageinfo/pageInfo.js

>+  getCellProperties: function(row, column, prop) {
>+    if (column.element.getAttribute("id") == "image-address")

Can you not check column.element.id instead?

(Unrelated to this patch, but it looks like columnids and colcount are unused in this file?)
Attachment #376617 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 3

9 years ago
Created attachment 377641 [details] [diff] [review]
Patch (v2)

(In reply to comment #2)
> (From update of attachment 376617 [details] [diff] [review])
> >diff --git a/browser/base/content/pageinfo/pageInfo.js b/browser/base/content/pageinfo/pageInfo.js
> 
> >+  getCellProperties: function(row, column, prop) {
> >+    if (column.element.getAttribute("id") == "image-address")
> 
> Can you not check column.element.id instead?

Yes.  Done in the new revision.  Also moved the check to the getCellProperties which actually gets used... :/

> (Unrelated to this patch, but it looks like columnids and colcount are unused
> in this file?)

Yeah, I think so.  I'll file a new bug on this.
Attachment #376617 - Attachment is obsolete: true
Attachment #377641 - Flags: review?(gavin.sharp)
Comment on attachment 377641 [details] [diff] [review]
Patch (v2)

Well that's confusing. Can you remove the unused getCellProperties, and cache both atoms used by the real one ("ltr" and "broken")?
(Assignee)

Comment 5

9 years ago
Created attachment 377644 [details] [diff] [review]
Patch (v3)

(In reply to comment #4)
> (From update of attachment 377641 [details] [diff] [review])
> Well that's confusing. Can you remove the unused getCellProperties, and cache
> both atoms used by the real one ("ltr" and "broken")?

The other getCellProperties gets used by the meta tags tree.

I added the code to cache "broke" in this version of the patch.
Attachment #377641 - Attachment is obsolete: true
Attachment #377644 - Flags: review?(gavin.sharp)
Attachment #377641 - Flags: review?(gavin.sharp)
(Assignee)

Comment 6

9 years ago
(In reply to comment #3)
> > (Unrelated to this patch, but it looks like columnids and colcount are unused
> > in this file?)
> 
> Yeah, I think so.  I'll file a new bug on this.

Bug 493225 filed.
Comment on attachment 377644 [details] [diff] [review]
Patch (v3)

>diff --git a/browser/base/content/pageinfo/pageInfo.js b/browser/base/content/pageinfo/pageInfo.js

>-    props.AppendElement(aserv.getAtom("broken"));
>+    props.AppendElement(aserv.getAtom(this._brokenAtom));

You're calling getAtom and passing an atom? Even if that works, it's not optimal!

I'd be a lot more comfortable reviewing patches if this functionality had some tests. Several of the patches for this bug appear to have been untested (original one didn't touch the right function, this one potentially breaks broken-styling, etc.)
Attachment #377644 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 8

8 years ago
Created attachment 446990 [details] [diff] [review]
Patch (v3.1)

(In reply to comment #7)
> (From update of attachment 377644 [details] [diff] [review])
> >diff --git a/browser/base/content/pageinfo/pageInfo.js b/browser/base/content/pageinfo/pageInfo.js
> 
> >-    props.AppendElement(aserv.getAtom("broken"));
> >+    props.AppendElement(aserv.getAtom(this._brokenAtom));
> 
> You're calling getAtom and passing an atom? Even if that works, it's not
> optimal!

Fixed.

> I'd be a lot more comfortable reviewing patches if this functionality had some
> tests. Several of the patches for this bug appear to have been untested
> (original one didn't touch the right function, this one potentially breaks
> broken-styling, etc.)

You're right, but I'm not sure how I can test this.  As a demo of how this patch looks, I took a screenshot of the Page Info window for this URL: data:text/html,<img src="http://www.google.com/"><img src="http://www.google.com/logo.png">

http://grab.by/4x6o

The platform level functionality landed in bug 478377 is tested in the reftest suite.  Maybe we should have Litmus tests here?
Attachment #377644 - Attachment is obsolete: true
Attachment #446990 - Flags: review?(gavin.sharp)
Attachment #446990 - Flags: review?(gavin.sharp) → review+
Comment on attachment 446990 [details] [diff] [review]
Patch (v3.1)

aserv is now unused, r=me with it removed.
(Assignee)

Comment 10

8 years ago
Created attachment 447007 [details] [diff] [review]
Patch to land
Attachment #446990 - Attachment is obsolete: true
(In reply to comment #8)
> The platform level functionality landed in bug 478377 is tested in the reftest
> suite.  Maybe we should have Litmus tests here?

What exactly do you want the litmus test to do (since I don't entirely understand what this patch does)?
(Assignee)

Comment 12

8 years ago
(In reply to comment #11)
> (In reply to comment #8)
> > The platform level functionality landed in bug 478377 is tested in the reftest
> > suite.  Maybe we should have Litmus tests here?
> 
> What exactly do you want the litmus test to do (since I don't entirely
> understand what this patch does)?

I don't particularly think that a Litmus test is necessary here, but I'll leave the final decision to Gavin.
(Assignee)

Comment 13

8 years ago
http://hg.mozilla.org/mozilla-central/rev/7edcbc1fb0ee
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a5
let's not worry about it.
Flags: in-litmus? → in-litmus-
You need to log in before you can comment on or make changes to this bug.