Pale Moon forum

Egh... I thought I'd figured it out, but the problem is still cropping up. Is a return value of "Maybe<AspectRatio>" different from a return value of "AspectRatio" for overloading?

I also noticed ScaleIntrinsicSizeForDensity in nsImageFrame.cpp was venturing into bug 1149357's territory... I guess I got a little too far ahead when copying code.

I backtracked on nsImageFrame to only implement the changes in the relevant bugs without getting into other territories, but I need to get a reference to HTMLImageElement using nsImageFrame.h's mContent. Firefox has a FromNode function for many DOM elements, but as far as I can tell, Pale Moon only has it in "ShadowRoot"? What's the correct functionality for getting a reference to the HTML*Element in this context?


Regarding GetIntrinsicRatio, it seems that results in generating which explains a lot. I don't know how this generated code stuff works, but is there even a way to get it to generate instead?

For now I rewrote everything to use the ByRef/return error methodology, just in case that's the only way Pale Moon structures attribute Gets.

However, now I'm stumbling into unknown territory with the nsCSSProp* classes. I'm assuming for now I can just use a "hidden" float css type to store and retrieve aspect ratios instead of actually dealing with an entirely new entry method of WWWxHHH because we're not implementing it as an active attribute just yet...

Is VARIANT_NUMBER sufficient for my purposes, or am I going to run into unexpected conversion issues? What about VARIANT_ANGLE(_OR_ZERO)? What would be better suited for these purposes? I'm using NUMBER for now, but if it should be something else, let me know.

I'm also not sure how much of the style property stuff I need to add to make this work while not being an actual property, but I'll try to muddle through.

Ok, well... it compiles, it runs, and it even seems to work. But it's nowhere near done.

The in-code questions I have marked in particular are:
How do I get an HTMLImageElement type reference from an nsImageFrame's mContent so I can check the IsAwaitingLoad() function I added?

I don't know what nsStyleStruct.cpp's DependsOnPositioningAreaSize() controls, but Firefox's version checks to see if both the width-type and height-type are auto, not just that they're equal to each other. Should this be changed?

And, for this one, I have a feeling the answer is more likely "leave it alone", but nsSVGOuterSVGFrame.cpp's GetIntrinsicRatio() in Firefox is set to use SVGAnimatedLength for the width and height values returned from content->mLengthAttributes, but Pale Moon's using nsSVGLength2. Another set of lines I didn't actually touch for this code, though I changed a decent amount that ends up using its variables.


Aside from those, there's this function and this set of functions that I mostly had to write myself with all the differences between browsers. And all the CSS Ruleset stuff is a literal stab in the dark at this point.


And lastly this mess definitely needs to be fixed before release to handle vertical writing mode, which Firefox still doesn't do, plus some discussion on type here from the original FF author of most of this code which bears noting. But I'll probably handle these last two things myself - no answers required from anyone else there.


Oh and I'll do some more tests, but I might have broken the LiveClick extension while I was working. Not sure how, but certain values that are not supposed to be null are ending up being null when I open the Bookmarks menu or any RSS Bookmark lists with LiveClick enabled. I haven't actually tried a straight release build to see if the issue was introduced in my code or just a result of a compiling difference or something (I'm using VS2019 because I already had it installed), but I'll try to narrow that down myself, as well. -- Edit: yep, something I added broke it.


Anyway, I think the "rough draft" could be considered done... ish. Please let me know all the places I screwed up horribly.

Regarding LiveClick crashing... I'm still trying to track down the cause, but it looks like some things are null that shouldn't be null while the menu relational trees are being constructed... nsCSSFrameConstructor's IsValidSibling check keeps getting sent a null aContent with a sibling that itself has a null mContent, and null mPrev/NextSibling values as well. aDisplay is set to UNSET_DISPLAY and parentType is menuFrame. It crashes when trying to determine the styleContext to get a real aDisplay, because aContent is null. That in turn seems to be getting called by FindSiblingInternal which is passing an aIter with a null mBindingParent, a null aTargetContent, and UNSET_DISPLAY. Which of course is coming from FindSibling, which gets its targetContent from calling aIter.Get() on the FlattenedChildIterator passed to it from... whatever insertion caused it. My guess is that I'm not setting something correctly by default for the AspectRatio value in CSS that LiveClick triggers accidentally, maybe by iterating through an object's style data? Which it shouldn't show up in because it's supposed to be a hidden attribute... There's enough difference between PM and FF with the object relations (Servo and all that), that without more direct knowledge on how Pale Moon wants these things to be done, I won't know how to give it everything it needs.

I think... I'm not sure, but I think it's tied to the PlacesViewBase.prototype._insertNewItemToPopup call being overwritten... When a new element is created and inserted before the last child of a menu item (LiveClick's LiveClickChrome.Browser.folderBeforeInsert() function), it triggers the crash. So I think that means that an AspectRatio property is probably not being created at the right point in time...

Anyway, I'm going to try a few more things, but I think this might be as far as I can do this without some help from experienced hands.

I thought I'd found the problem when I noticed a line I deleted from layout\generic\nsFlexContainerFrame.cpp wasn't supposed to be deleted, but I was wrong. I wish I could compile and test Firefox's code right after these changes were added, but of course by then they had long abandoned XUL extensions, so LiveClick wouldn't work anyway. I was also thinking I could add breakpoints to all the code I added, but if the problem is, as I suspect, due to an omission somewhere, then that would be useless anyway. And I've tried searching everywhere for places where CSS attributes are managed and included, and it looks like all the places that I skipped are for truly existing CSS attributes, not hidden internal ones.

I wish I could add changes bit by bit, but since they're basically all to facilitate a transition of types, intermediate steps won't compile or run at all. I'll try to break it down a bit to see if any of the code that can be separately compiled fixes the issue by removal, but I don't have very high hopes there, either. And since it takes three hours to compile PM on my laptop, going's gonna be slow.

I was able to narrow it down a little, by just implementing the very first changes from D29244 first, which happens to be enough to trigger the issue. So my problem lies somewhere in here. Of course, this means all my calls to AspectRatio() from imgIContainer.idl which I left as noscript instead of notxpcom, nostdcall (because as I mentioned earlier, I couldn't find a way to get Pale Moon to turn the ByRef param attribute request into a Maybe<>-able function like FF uses) are part of the possible problem-causing changes, as is a decent bulk of the code, all in all. So "narrow" isn't quite the right word. But it does remove a lot of the code that I wrote from the places to look. There is also the whole layout/painting/nsImageRenderer -> layout/base/nsCSSRendering thing... but the functions have the same names and comments, and even the same nsImageRenderer namespace, so I'm assuming Firefox split them into separate files some time after the fork, and that it's still all the right place to apply those changes.

The more I look into this, the more confused I get. nsCSSFrameConstructor.cpp's GetInsertionPrevSibling calls iter.Seek on aChild, when aChild is the item to be added, which as best I can tell means it should already be added to the menu item by that point, but stepping through Seek in ChildIterator.h reveals no matches to aChildToFind, as set in NodeBinding.cpp::insertBefore's arg0(LiveClick's observer), though it's definitely iterating through arg1 (LiveClick's aElement.lastChild)'s parent as it should, including listing arg1 itself. So for some reason, LiveClick's observer element is... not there yet. Or was cloned somewhere along the way. But I'm watching the exact pointer be added via InsertChildAt on line 1643 of nsINode.cpp, so I don't get it.

And I really don't get how changing a return type could have caused any of this.

Ah hell, it's not my code at all - it's Master. Some change in the official Pale Moon trunk is causing the problem LiveClick is exposing.

Just double-checked by applying my changes to Release instead of Master - no crash. What a waste of three days.

RealityRipple wrote: ↑

2020-07-22, 04:44

The in-code questions I have marked in particular are:
How do I get an HTMLImageElement type reference from an nsImageFrame's mContent ...?

You can probably go the route of mContent->GetComposedDoc()->GetElementById({{key}}) or mContent->GetUncomposedDoc()->GetElementById({{key}}) or similar. mContent is the generic content holder wrapping the document (and other parts not important here) which in turn will contain the element you seek.

I don't know what nsStyleStruct.cpp's DependsOnPositioningAreaSize() controls, but Firefox's version checks to see if both the width-type and height-type are auto, not just that they're equal to each other. Should this be changed?

Sounds like it should be, yes. Not that width and height being auto makes much sense to specify at the same time, but i guess if Chrome handles it, then it'd have been used in the wild.

And, for this one, I have a feeling the answer is more likely "leave it alone", but nsSVGOuterSVGFrame.cpp's

It's unrelated to this change so leave it alone just for that already. If you want to look at why that was changed and if it's a good idea for us later, you can do so in another issue.. Please stay focused on this alone

As for the rest I'd really have to look when I have time (since I doubt anyone else will be able to review this meaningfully?) I mean, I'll have to do a lot of research into what i'm looking at myself as well (since contrary to popular belief I don't know everything about the entire codebase, not even the layout part) and it seems that's still not something many other volunteers are doing/willing to do; but ultimately if it compiles, works to spec, and doesn't break anything else, then that's the major sticking points.

Moonchild wrote: ↑

2020-07-29, 08:36

As for the rest I'd really have to look when I have time (since I doubt anyone else will be able to review this meaningfully?) I mean, I'll have to do a lot of research into what i'm looking at myself as well (since contrary to popular belief I don't know everything about the entire codebase, not even the layout part) and it seems that's still not something many other volunteers are doing/willing to do; but ultimately if it compiles, works to spec, and doesn't break anything else, then that's the major sticking points.

I totally understand, the code for this project is huge. And actually, regarding different issues and focus, should I split my current work into two issues and pull requests? The conversion of internal AspectRatio from nsSize to float seems like it should be separated from the actual intrinsic ratio changes, especially for future search/backout/cherrypick stuff, just as they were in FF's handling of it.

Sounds like a plan.

Landed in PR #1622 and PR #1613. Thank you for your patience while I stumbled through this netscape.