Re: [webkit-dev] [webkit-changes] [264332] trunk/Source

2020-07-14 Thread Said Abou-Hallawa


> On Jul 14, 2020, at 10:40 AM, Darin Adler  wrote:
> 
> We need to be cautious about adding header includes to other headers. Adding 
> includes to .cpp files is likely fine and not a deeply consequential 
> decision, but adding to headers is something that can have a massive effect 
> on build times over time.
> 
>> On Jul 13, 2020, at 10:44 PM, hironori.fu...@sony.com 
>>  wrote:
>>  <>Modified: trunk/Source/WebCore/platform/graphics/ColorSerialization.h 
>> (264331 => 264332)
>> 
>> --- trunk/Source/WebCore/platform/graphics/ColorSerialization.h  
>> 2020-07-14 05:17:20 UTC (rev 264331)
>> +++ trunk/Source/WebCore/platform/graphics/ColorSerialization.h  
>> 2020-07-14 05:44:25 UTC (rev 264332)
>> @@ -25,6 +25,8 @@
>>  
>>  #pragma once
>>  
>> +#include 
> This change is wrong. The header to include here is Forward.h, not 
> WTFString.h. Not super-harmful since WTFString.h is already so widely 
> included, but we need to be really cautious in headers.
> 
>> Modified: trunk/Source/WebCore/svg/SVGParserUtilities.h (264331 => 264332)
>> 
>> --- trunk/Source/WebCore/svg/SVGParserUtilities.h2020-07-14 05:17:20 UTC 
>> (rev 264331)
>> +++ trunk/Source/WebCore/svg/SVGParserUtilities.h2020-07-14 05:44:25 UTC 
>> (rev 264332)
>> @@ -24,6 +24,7 @@
>>  #include "ParsingUtilities.h"
>>  #include 
>>  #include 
>> +#include 
> Same mistake.
> 
> I’d really like to come up with some other system for reviewing adding 
> headers to *headers*.

I looked at the SVGParserUtilities.h and in fact we do not have to include 
HashSet.h or Vector.h either. HashSet and Vector need only to be forward 
declare in this header because they are referenced as return values. So 
 can replace all the three header files.


Maybe the policy can be: include  before trying to include any 
other wtf header file. 

I think we need to include the wtf header files only when the compiler needs to 
know the size. For example:

class X {
Vector m_list;
};

Thanks,
Said

> 
> — Darin
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


[webkit-dev] Unified source builds and adding or removing files...

2019-03-19 Thread Said Abou-Hallawa
I have been working on patches that require adding and removing cpp files from 
WebCore/Sources.txt. Almost every time I add or remove a file, I hit undefined 
symbol compilation error in some unrelated source or header file. Because a 
group of source files are compiled in one unified source file, some 
dependencies get hidden.  The same symbol is defined in another source or 
header file. Once sources are moved to different unified sources, the problem 
gets uncovered.

For example the file svg/graphics/filters/filters/SVGFEImage.h uses the class 
TreeScope without forward declaring it or including its header file. Oddly the 
source file svg/graphics/filters/filters/SVGFEImage.cpp compiles in the trunk 
right now. If a file is added to or removed from WebCore/Sources.txt such that 
this source is moved to another unified source, the compiler will give an error 
that TreeScope is not defined.

Another example is  which fixes a 
compilation error on GTK port. The same file was compiling fine on macOS but it 
failed on GTK.

Can we fix this issue? The fixes for such errors look very mysterious in the 
patches. It can also cause build breaks because the ports do not compile the 
same files.

One naive solution is to have the EWS bots compile without the unified source 
builds. In this case, all the header and source files must have the required 
forward declaration and/or the header file inclusion. So adding or removing 
files should not affect the ability to compile any other source file.

Thanks,
Said

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


Re: [webkit-dev] Code Style: Opinion on returning void

2019-02-20 Thread Said Abou-Hallawa


> On Feb 20, 2019, at 10:35 AM, Mark Lam  wrote:
> 
> I also prefer it, and I think some coding patterns may require it e.g. in 
> templates where sometimes we want to specialize into a void function, and 
> other times into a function that returns a value.  However, this is rarely 
> needed in practice.  Without being able to return void, writing such a 
> template will be a pain if not impossible.

But the template and the macro cases are different. The void return in this 
case is implicit and you do not recognize it unless you see the caller.

template
auto caller(const Functor& functor)
{
return functor();
}

Nothing in the return statement above says it will return void. But if you pass 
it a pointer to a returning void function it will. So outside the scope of 
macros and templates, I think returning void is not nor really needed.

> 
> Mark
> 
>> On Feb 20, 2019, at 7:26 AM, Saam Barati > > wrote:
>> 
>> I prefer it as well.
>> 
>> - Saam
>> 
>> On Feb 20, 2019, at 6:58 AM, Chris Dumez > > wrote:
>> 
>>> I also prefer allowed returning void. 
>>> 
>>> Chris Dumez
>>> 
>>> On Feb 19, 2019, at 10:35 PM, Daniel Bates >> > wrote:
>>> 
 
 
 On Feb 19, 2019, at 9:42 PM, Ryosuke Niwa >>> > wrote:
 
> On Tue, Feb 19, 2019 at 8:59 PM Daniel Bates  > wrote:
> > On Feb 7, 2019, at 12:47 PM, Daniel Bates  > > wrote:
> >
> > Hi all,
> >
> > Something bothers me about code like:
> >
> > void f();
> > void g()
> > {
> > if (...)
> > return f();
> > return f();
> > }
> >
> > I prefer:
> >
> > void g()
> > {
> > if (...) {
> > f();
> > return
> > }
> > f();
> > }
> >
> Based on the responses it seems there is sufficient leaning to codify
> the latter style.
> 
> I don't think there is a sufficient consensus as far as I can tell. Geoff
 
 I didn't get this from Geoff's remark. Geoff wrote:
 
 ***“return f()” when f returns void is a bit mind bending.***
 Don't want to put words in Geoff's mouth. So, Geoff can you please 
 confirm: for the former style, for the latter style, no strong opinion.
 
> and Alex both expressed preferences for being able to return void,
 
 I got this from Alex's message
 
> and Saam pointed out that there is a lot of existing code which does this.
 
 I did not get this. He wrote emphasis mine:
 
 I've definitely done this in JSC. ***I don't think it's super common***, 
 but I've also seen code in JSC not written by me that also does this.
 
> Zalan also said he does this in his layout code.
 
 I did not get this, quoting, emphasis mine:
 
 I use this idiom too in the layout code. I guess I just prefer a more
 compact code.
 ***(I don't feel too strongly about it though)***
 
 By the way, you even acknowledged that "WebKit ... tend[s] to have a 
 separate return.". So, I inferred you were okay with it. But from this 
 email I am no longer sure what your position is. Please state it clearly.
 
> - R. Niwa
> 
 ___
 webkit-dev mailing list
 webkit-dev@lists.webkit.org 
 https://lists.webkit.org/mailman/listinfo/webkit-dev 
 
>>> ___
>>> webkit-dev mailing list
>>> webkit-dev@lists.webkit.org 
>>> https://lists.webkit.org/mailman/listinfo/webkit-dev 
>>> 
>> ___
>> webkit-dev mailing list
>> webkit-dev@lists.webkit.org 
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
> 
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


[webkit-dev] Image decoding refactoring...

2016-09-10 Thread Said Abou-Hallawa
Can someone on the GTK or the EFL ports review this patch: 
https://bugs.webkit.org/attachment.cgi?id=288475=review? 
 It is a work 
towards the asynchronous image decoding task: 
https://bugs.webkit.org/show_bug.cgi?id=155322 
. The goal is have one 
declaration for the class ImageDecoder instead of having two declarations in 
ImageDecoder.h and ImageDecoderCG.h. And also to replace FrameData in 
BitmapImage.h and ImageFrame in ImageDecoder.h by one class/struct. Currently 
for non CG ports the image frame data is cached twice: one in ImageDecoder 
object and the other in BitmapImage object. If ImageDecoder and BitmapImage use 
the same class/struct for the image frame, no need to cache it twice.

Also I could get this patch to compile but I could not verify it passes the 
layout tests. Is there a way to run the layout tests on the GTK/EFL EWS? I do 
not want to land the patch and then know that the layout tests did not pass and 
I have to roll the patch out. Any help in verifying the layout tests is very 
appreciated.

Thanks,
Said

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


Re: [webkit-dev] Memory leak tracking in WebKit

2016-01-05 Thread Said Abou-Hallawa
This seems to be a reference cycle between SVGAnimatedListPropertyTearOff and 
SVGListPropertyTearOff. In SVGAnimatedListPropertyTearOff::animVal(), m_animVal 
is assigned to a new Ref but this new Ref increments 
the refcount of this. This looks similar to 
https://bugs.webkit.org/show_bug.cgi?id=151810.

> On Jan 5, 2016, at 2:19 PM, Vienneau, Christopher  wrote:
> 
> Thanks for suggesting that Simon, I’ve now opened the bug:
> https://bugs.webkit.org/show_bug.cgi?id=152759 
> 
>  
> Chris
>  
> From: simon.fra...@apple.com [mailto:simon.fra...@apple.com] 
> Sent: Tuesday, January 05, 2016 12:09 PM
> To: Vienneau, Christopher 
> Cc: WebKit Development 
> Subject: Re: [webkit-dev] Memory leak tracking in WebKit
>  
> This sounds like a bug that would affect all WebKit ports. Can you file a 
> bugs.webkit.org  bug, and continue investigation 
> there?
>  
> Simon
>  
> On Jan 5, 2016, at 12:03 PM, Vienneau, Christopher  > wrote:
>  
> Hi,
>  
> I’ve resumed the memory leak tracking I was doing last year, I have some more 
> details to share, hopefully you’ll be able to suggest how I might fix it.  
> The source of the leak appears to come from the below callstack.  A cache of 
> animation points is being created in SVGAnimatedProperty(SVGElement* 
> contextElement, const QualifiedName& attributeName, AnimatedPropertyType 
> animatedPropertyType), however the destructor for SVGAnimatedProperty is 
> never called.  The passed in contextElement gains a ref when the 
> SVGAnimatedProperty is created, however I’m not seeing a code path where the 
> animation points should be destroyed.  This effects both svg polyline and 
> polygon, and results in leaking the whole page.
>  
> Thanks for any help you can provide,
>  
> Chris Vienneau
>  
>  
> \WebCore\svg\properties\SVGAnimatedProperty.cpp
> SVGAnimatedProperty::SVGAnimatedProperty(SVGElement* contextElement, const 
> QualifiedName& attributeName, AnimatedPropertyType animatedPropertyType)
> : m_contextElement(contextElement)
> , m_attributeName(attributeName)
> , m_animatedPropertyType(animatedPropertyType)
> , m_isAnimating(false)
> , m_isReadOnly(false)
> {
> }
>  
> > 
> > EAWebKitd.dll!WebCore::SVGAnimatedProperty::SVGAnimatedProperty(WebCore::SVGElement
> >  * contextElement, const WebCore::QualifiedName & attributeName, 
> > WebCore::AnimatedPropertyType animatedPropertyType) Line 29
> > C++
> 
> EAWebKitd.dll!WebCore::SVGAnimatedListPropertyTearOff::SVGAnimatedListPropertyTearOff(WebCore::SVGElement
>  * contextElement, const WebCore::QualifiedName & attributeName, 
> WebCore::AnimatedPropertyType animatedPropertyType, WebCore::SVGPointList & 
> values) Line 166 C++
> 
> EAWebKitd.dll!WebCore::SVGAnimatedListPropertyTearOff::create(WebCore::SVGElement
>  * contextElement, const WebCore::QualifiedName & attributeName, 
> WebCore::AnimatedPropertyType animatedPropertyType, WebCore::SVGPointList & 
> values) Line 159 C++
> 
> EAWebKitd.dll!WebCore::SVGAnimatedProperty::lookupOrCreateWrapper(WebCore::SVGPolyElement
>  * element, const WebCore::SVGPropertyInfo * info, WebCore::SVGPointList & 
> property) Line 57 C++
>
> EAWebKitd.dll!WebCore::SVGPolyElement::lookupOrCreatePointsWrapper(WebCore::SVGElement
>  * contextElement) Line 117C++
>EAWebKitd.dll!WebCore::SVGPolyElement::animatedPoints() Line 
> 130  C++
>
> EAWebKitd.dll!WebCore::updatePathFromPolylineElement(WebCore::SVGElement * 
> element, WebCore::Path & path) Line 106   C++
>
> EAWebKitd.dll!WebCore::updatePathFromGraphicsElement(WebCore::SVGElement * 
> element, WebCore::Path & path) Line 172   C++
>
> EAWebKitd.dll!WebCore::RenderSVGShape::updateShapeFromElement() Line 84   
> C++
>EAWebKitd.dll!WebCore::RenderSVGPath::updateShapeFromElement() 
> Line 48  C++
>EAWebKitd.dll!WebCore::RenderSVGShape::layout() Line 164   C++
>
> EAWebKitd.dll!WebCore::SVGRenderSupport::layoutChildren(WebCore::RenderElement
>  & start, bool selfNeedsLayout) Line 281   C++
>EAWebKitd.dll!WebCore::RenderSVGRoot::layout() Line 181  
> C++
>EAWebKitd.dll!WebCore::RenderElement::layoutIfNeeded() Line 
> 135C++
>EAWebKitd.dll!WebCore::RenderBlockFlow::layoutLineBoxes(bool 
> relayoutChildren, WebCore::LayoutUnit & repaintLogicalTop, 
> WebCore::LayoutUnit & repaintLogicalBottom) Line 1621   C++
>
> EAWebKitd.dll!WebCore::RenderBlockFlow::layoutInlineChildren(bool 
> 

Re: [webkit-dev] Painting /Drawing mechanism in webkit

2015-02-03 Thread Said Abou-Hallawa
In CachedImage::finishLoading(), the SharedBuffer parameter should have the raw 
data of the image. The data in this buffer is the content of the image file.  
BitmapImage::dataChanged() might be a better place to alter the image data if 
you want your solution to be specific to bitmap images.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


[webkit-dev] Tagged assertions...

2014-11-06 Thread Said Abou-Hallawa
Recently I ran into an issue where I wanted to find if there is a bug filed 
against a certain assertion. The only way I was able to use is to search for 
the name of the function which has the assertion.  The function can be 
overloaded or the function can have more than one assertion.  So I found it 
difficult to track the assertion I am interested in.  In future if the function 
gets renamed, it will be even more difficult.

I have a suggestion for this.

A pre-build tool will tag every new assertion after the change is committed.  
For example in the code I will add the following assertion:

ASSERT(condition);

The pre-build tool will change it to be:

ASSERT_TAG(condition, “abcde”);

where “abcde” is a unique tag in all the source code.  A database should be 
maintained by the build process to keep track what tags are available to be 
assigned.

When the assertion fires, the dump may look like the following:

ASSERTION FAILED (tag: abcde) condition
… rest of the call-stack

In Bugzilla, there should be a field for the assertion tag.  If I want to file 
an assertion bug, I should be filling this field like the following:

Product:WebKit
Summary:ASSERTION failed in someFunction
Assertion tag:  abcde
Description:
Attachment:

Later if the assertion fires, from the dump I can know exactly what assertion 
fired.  And more importantly I can search for it easily.  In the Bugzilla 
search page, I should see a field for the assertion tag in the simple search 
page:

Status: All
Product:WebKit
Words:
Assertion tag:  abcde

This search will bring me all the opened bugs in WebKit which has the assertion 
tag = “abcde” if there is any.

ID  Product CompAssigneeStatus  
Resolution  Assertion tag   Summary
12345   WebKit  
abcde   ASSERTION failed in 
someFunction

Thanks,
Said

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


Re: [webkit-dev] Tagged assertions...

2014-11-06 Thread Said Abou-Hallawa
The function name and the line number may change if the code changes.   A 
unique tag associated with the assertion should stay the same.  So when you 
want to search for the assertion, you just need the tag since it is unique.  We 
can even extend the dump to do the search in Bugzilla as well.  So if the 
assertion fires, the dump might look like the following:

ASSERTION FAILED (tag: abcde) condition
Bug 12345 (closed): ASSERTION failed in someFunction
Bug 67890 (open): Still ASSERTION failed in someFunction
… rest of the call-stack

So you do not even need to search Bugzilla for it.  You would know immediately 
that the assertion is being tracked by an opened bug.___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


Re: [webkit-dev] Tagged assertions...

2014-11-06 Thread Said Abou-Hallawa
These assertions can’t be uniquely identified if they are repeated in the same 
function

ASSERT(false);
ASSERT_NOT_REACHED();

And more importantly the same function can have the same assertion repeated in 
multiple places 

void function()
{
int result = 0;

result += someFunction();
ASSERT(result  0);
result += anotherFunction();
ASSERT(result  0);
}

I am not saying we have a problem that we have to fix.  I am just saying if we 
need to uniquely identify the bugs related to a specific assertion easily or we 
need to automate the linkage between the assertions and Bugzailla, we need tags 
for the assertions.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


Re: [webkit-dev] Tagged assertions...

2014-11-06 Thread Said Abou-Hallawa
When the logical expression in the ASSERT is changed, you should do the 
following:

-   ASSERT_TAG(condition, “abcde”);
+  ASSERT(new_condition);

Then the pre-build tagging tool will generate a new tag for the new assertion:

+  ASSERT(new_condition, “fghij”);

So the assertions do not get messed up with Bugzailla.

The pre-build tool runs periodically only on Bot, maybe once every day and only 
on the touched files since it was last run.  Then it commits only the changed 
files because of tagging.  This should not add much to the build process.

Both ASSERT and ASSERT_TAG should be supported.  Since the developer should 
only add ASSERT and leave the tagging for the pre-build tagging tool.

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


Re: [webkit-dev] Tagged assertions...

2014-11-06 Thread Said Abou-Hallawa
Are you saying when an assertion fires I have to do the following?

1. Search Bugzilla for the assertion and if it is no found, file a new one
2. Submit a change just for adding a FIXME comment just above the assertion 
with new bug filed

Would not be easier to just have the tag work both ways?  You can search 
Bugzilla and the source code for the tag.



___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


[webkit-dev] gtk failure

2014-10-31 Thread Said Abou-Hallawa
I submitted a patch for the bug https://bugs.webkit.org/show_bug.cgi?id=137132 
https://bugs.webkit.org/show_bug.cgi?id=137132
The build failed for gtk-wk2 but passed for mac and mac-wk2.  Here is the 
failure.

Last 500 characters of output:
e/CMakeFiles/WebCore.dir/rendering/style/RenderStyle.cpp.o -c 
../../Source/WebCore/rendering/style/RenderStyle.cpp
../../Source/WebCore/rendering/style/RenderStyle.cpp: In member function 
'WebCore::Color WebCore::RenderStyle::colorIncludingFallback(int, bool) const':
../../Source/WebCore/rendering/style/RenderStyle.cpp:1525:10: error: 
'CSSPropertyWebkitColumnRuleColor' was not declared in this scope
 case CSSPropertyWebkitColumnRuleColor:

In the patch I have the following change 

Source/WebCore/rendering/style/RenderStyle.cpp 
http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/style/RenderStyle.cpp
@@Color RenderStyle::colorIncludingFallbac
15221522case CSSPropertyOutlineColor:
15231523result = visitedLink ? visitedLinkOutlineColor() : 
outlineColor();
15241524break;
1525 case CSSPropertyWebkitColumnRuleColor:
 1525case CSSPropertyColumnRuleColor:
15261526result = visitedLink ? visitedLinkColumnRuleColor() : 
columnRuleColor();
15271527break;
15281528case CSSPropertyWebkitTextDecorationColor:


I am un-prefixing the column rule color property.  So I replaced 
CSSPropertyWebkitColumnRuleColor with CSSPropertyColumnRuleColor.  But the 
failure is saying the prefixed property is not defined.  

How can for gtk-wk2 only, the patch is not applied in RenderStyle.cpp but is 
applied with other files and is applied also for all files for the mac platform?

Thanks,
Said___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev