Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#6189 closed Bug (fixed)

Reducing the code size

Reported by: Sa'ar Zac Elias Owned by: Sa'ar Zac Elias
Priority: Normal Milestone: CKEditor 3.4.2
Component: General Version:
Keywords: Doc? Cc:

Description (last modified by Sa'ar Zac Elias)

A part of a discussion with Fred:

Clearly the code is constantly getting bigger. So how about reviewing the code and try to reduce it: use 1 and 0 for true and false when possible, remove the "defined" configuration when possible (leaving just comments about them of course) etc.

Attachments (2)

6189.patch (107.1 KB) - added by Sa'ar Zac Elias 14 years ago.
6189_2.patch (114.7 KB) - added by Sa'ar Zac Elias 14 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 14 years ago by Sa'ar Zac Elias

Description: modified (diff)
Status: newassigned

comment:2 Changed 14 years ago by Alfonso Martínez de Lizarrondo

I think that all the true/false -> 1/0 conversions should be done at package time. That way the code remains as is and it will be future-proof.

comment:3 Changed 14 years ago by Frederico Caldeira Knabben

@alfonsoml, I think we're using the strict equality operator in some places, which would break things. Maybe better to have the 1/0 thing is a coding style best practice, so we have better control of it.

Changed 14 years ago by Sa'ar Zac Elias

Attachment: 6189.patch added

comment:4 Changed 14 years ago by Sa'ar Zac Elias

Status: assignedreview

These changes should become routine for the core developers as well as the contributers, so adding documentation regarding flags and initializing of config settings is recommended IMHO.

comment:5 Changed 14 years ago by Garry Yao

Isn't this suppose to be a [MicroChanges micro-change]?

comment:6 Changed 14 years ago by Sa'ar Zac Elias

That was my initial thought, but Fred asked me to open a ticket for that and put the patch on review.

comment:7 Changed 14 years ago by Garry Yao

Status: reviewreview_failed

In which sense does the lazy config option initialization style contribute to performance? Anway some of the styles configs are also used by 'pastefromword' plugin thus get broken. Besides, some of the changes in the patch are CKPackager's task instead, e.g. quotes transformation, simple block braces, check this out.

comment:8 Changed 14 years ago by Garry Yao

For me this's pretty much a packager ticket really, the 1st feature I would vote would be an invariant variable feature that caches reference like editor.lang.common.generalTab will contribute a lot to the reduction.

Changed 14 years ago by Sa'ar Zac Elias

Attachment: 6189_2.patch added

comment:9 in reply to:  7 ; Changed 14 years ago by Sa'ar Zac Elias

Status: review_failedreview

Replying to garry.yao:

Besides, some of the changes in the patch are CKPackager's task instead, e.g. quotes transformation, simple block braces, check this out.

These are just some coding style fixes I added.

the 1st feature I would vote would be an invariant variable feature that caches reference like editor.lang.common.generalTab will contribute a lot to the reduction.

I agree, that could be great, but let's do it ourselves in the meanwhile.

comment:10 in reply to:  9 Changed 14 years ago by Garry Yao

the 1st feature I would vote would be an invariant variable feature that caches reference like editor.lang.common.generalTab will contribute a lot to the reduction.

I agree, that could be great, but let's do it ourselves in the meanwhile.

Moved to #6475, so don't worry about it now.

comment:11 in reply to:  5 Changed 14 years ago by Garry Yao

Status: reviewreview_passed

1kb of footprint reduction is produced after the patch.

comment:12 Changed 14 years ago by Sa'ar Zac Elias

Keywords: Doc? added
Resolution: fixed
Status: review_passedclosed

Fixed with [5949]. Additional minor fixes should be commited as micro changes.
These small things should become routine, let's have a documentation for them.

comment:13 Changed 14 years ago by Sa'ar Zac Elias

Milestone: CKEditor 3.5CKEditor 3.4.2

comment:14 Changed 14 years ago by Frederico Caldeira Knabben

A note... it was wrong to have such big change in a micro release. That's why it has been targeted to the 3.5 earlier. More attention is required in the future.

Another note... just to remember: the "this" trick is not needed. CKPackager automatically replaces "this" if there are enough occurrences of it to compensate it. Anyway, there is no need to change what has already been changed.

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy