search

OlgaTPark / tenfourfox

Mozilla for Power Macintosh.
http://www.tenfourfox.com/
Other
31 stars 0 forks source link

Migration to FPR2 #4

Open OlgaTPark opened 5 years ago

OlgaTPark commented 5 years ago

Additional patches from Firefox ESR52 for FPR2 on Intel

This bug documents things took from https://github.com/classilla/tenfourfox/issues/416.

Not taking:
Others:

Note for those who will be ever interested in what I do:
Some portions of code specific for Mac OS X 10.7 and 10.8 will be disabled in all PowerPC versions of TenFourFox and nonessential ones in TenFourFox and TenFiveFox. TenSixFox will keep those OS-specific code snippets.

OlgaTPark commented 5 years ago

@classilla,
Sorry to bother but when adding https://github.com/classilla/tenfourfox/commit/d9876efcf1bd00af751fc611df3e1c22d1f1d67b#diff-a16d499f18bb4d9858d33b2b2afa545c (Patching Mozilla Bug 1365189) to my fork, I saw that there's a (subtle — yes I'm paranoïd too) difference with the Mozilla bugfix.
The assigment of paintServerFrame to mPaintServerFrame when paintServerFrame isn't NULL is missing in https://github.com/OlgaTPark/tenfourfox/blob/d9876efcf1bd00af751fc611df3e1c22d1f1d67b/layout/base/nsCSSRendering.cpp#L4805-L4820 Versus Mozilla's version:

       // If the referenced element is an <img>, <canvas>, or <video> element,
       // prefer SurfaceFromElement as it's more reliable.
       mImageElementSurface =
         nsLayoutUtils::SurfaceFromElement(property->GetReferencedElement());
       if (!mImageElementSurface.GetSourceSurface()) {
-        mPaintServerFrame = property->GetReferencedFrame();
-        if (!mPaintServerFrame) {
+        nsIFrame* paintServerFrame = property->GetReferencedFrame();
+        // If there's no referenced frame, or the referenced frame is
+        // non-displayable SVG, then we have nothing valid to paint.
+        if (!paintServerFrame ||
+            (paintServerFrame->IsFrameOfType(nsIFrame::eSVG) &&
+             !paintServerFrame->IsFrameOfType(nsIFrame::eSVGPaintServer) &&
+             !static_cast<nsISVGChildFrame*>(do_QueryFrame(paintServerFrame)))) {
           mPrepareResult = DrawResult::BAD_IMAGE;
           return false;
         }
+        mPaintServerFrame = paintServerFrame;
       }

       mPrepareResult = DrawResult::SUCCESS;
       break;

This has the side effect that SVG images referred by background: -moz-element(#something); aren't rendered (that's the case of TenFourFox G3 version FPR2 and FPR15 (rosetta) — other SVG images referenced inside <img> tags are unaffected).
The following HTML code can be used as a testcase: 

<html>
    <head>
        <meta charset="UTF-8" />
    </head>
    <body>
        <img id="id20" width="100px" xmlns="http://www.w3.org/2000/svg" 
            src="https://upload.wikimedia.org/wikipedia/commons/0/02/SVG_logo.svg">
        </img>
        <img id="id21" width="100px" 
            src="https://en.wikipedia.org/static/images/project-logos/enwiki.png">      
        </img>
        <p style="background: -moz-element(#id20);"><br/><br/><br/></p>
        <p style="background: -moz-element(#id21);"><br/><br/><br/></p>
    </body>
</html>

Which has to render both the SVG logo (in SVG) and the Wikipedia logo (in PNG, for comparison). When I add this assignment to my fork, the SVG image is rendered (that's also the case of TenFourFox G3 45.9 (rosetta) and Firefox Legacy 67).
So (to conclude my uselessly long message), I'm wondering if this assignment is unintentionally missing (causing a specific rendering bug) or if you removed it for some PowerPC-specific reason?

[Funnier part]: I also intend to put in my fork a workaround for PDF fonts. It's an additional about:config setting (pdfjs.display.use_document_fonts) that disables both document fonts and downloadable fonts in PDFJS for TenFourFox and TenFiveFox (and not in the whole browser as browser.display.use_document_fonts does). This makes rendering of nearly every PDF less hurting for eyes (I'm using it for months and I won't revert it 😎). Technically speaking, it affects https://dxr.mozilla.org/mozilla-esr45/source/browser/extensions/pdfjs/content/PdfStreamConverter.jsm#359.
If you're interested, I can pull-request it to you and/or add a checkbox in about:preferences#tenFourFox (to be more user-friendly and less about:config-friendly`) (NOTE: I haven't committed the corresponding changeset yet now it is).

classilla commented 5 years ago

No, that's probably just an oversight on my part. I think that line should be restored. If it is, does that fix the problem? (My G5 is building the FPR16 beta so I can't easily check right this second.)

classilla commented 5 years ago

As far as the PDF font workaround, I'm not opposed to the idea and it sounds like a good one, but I probably won't expose it in the UI (just about:config) because of the need to translate the string for all the langpacks.

OlgaTPark commented 5 years ago

I (just) committed the changes for the discussed PDFJS setting (on a separate branch for easier pull requests).

Concerning the missing assignment, yes restoring it solves the bug (NOTE: I haven't committed the change anywhere).